Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declutter example notebook #83

Merged
merged 37 commits into from
Jan 18, 2024
Merged

Declutter example notebook #83

merged 37 commits into from
Jan 18, 2024

Conversation

ashjbarnes
Copy link
Collaborator

@ashjbarnes ashjbarnes commented Nov 15, 2023

Here I've removed all descriptions of Gadi / NCI and almost all of Payu from the reanalysis_forced notebook, and deleted the model forced one. This will be migrated to cosima recipes

The old notebook cells for "setup era5" and "set up run directory" are now methods of the experiment class which further streamlines the notebook. The using_payu flag is included here to give users the option of a config.yaml file being generated for their experiment. It defaults to False.

The default input directory has also been decluttered to make things generic and not Gadi-centric

Finally, to address the deprecation of the motu client (issue #79 ) I've removed this entirely and given users step by step instructions for using the GUI on Copernicus's website to download the required data.

Related to #59 in that I've updated the model-forced example but I won't delete it until we find a good home for it elsewhere (see COSIMA/cosima-recipes#308 )

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 92 lines in your changes are missing coverage. Please review.

Comparison is base (9388117) 72.97% compared to head (5fea3a0) 61.02%.

Files Patch % Lines
regional_mom6/regional_mom6.py 9.80% 92 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #83       +/-   ##
===========================================
- Coverage   72.97%   61.02%   -11.95%     
===========================================
  Files           3        3               
  Lines         444      544      +100     
===========================================
+ Hits          324      332        +8     
- Misses        120      212       +92     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ashjbarnes
Copy link
Collaborator Author

I'm going to update / fix the 'model forced' one then re-open this PR. Until we find a landing place for it I'll at least iron out some of the issues pointed out by Luwei

@ashjbarnes ashjbarnes closed this Nov 16, 2023
@ashjbarnes ashjbarnes reopened this Nov 16, 2023
@ashjbarnes
Copy link
Collaborator Author

I've re-added the ACCESS-OM2 example but made it clear in the markdown that it's intended for NCI users. This should be migrated elsewhere in COSIMA when we agree on the space

@ashjbarnes
Copy link
Collaborator Author

Codecov is down because I've moved the process era5 functionality to the package. A future PR that expands testing should address this rather than me making this PR longer. I've manually tested both notebooks and run the corresponding MOM6 experiments they produce

regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
@ashjbarnes
Copy link
Collaborator Author

@angus-g latest commit that re-introduces yaml for string manipulations fails on example notebooks. Could we please not make any commits that fail for the example notebooks? Ideally this would have been caught by unit testing that's not implemented yet, but the execution of the example notebooks start to finish are a good way to ensure commits aren't code breaking

I'm currently using a branch from declutter_notebook for my own research, which will eventually be the next PR incorporating tides into the workflow, hence this branch needs to remain functional.

I think it would just be easiest if the string manipulation stuff is dealt with in a separate PR, as it's beyond the scope of what this PR intended to do (i.e, migrate existing code from the example notebooks into the codebase)

The error was ultimately: RepresenterError: cannot represent an object: [/scratch/v45/ab8992/regional_mom6_configs/tasmania-example](https://file+.vscode-resource.vscode-cdn.net/scratch/v45/ab8992/regional_mom6_configs/tasmania-example)

@angus-g
Copy link
Collaborator

angus-g commented Dec 6, 2023

Apologies, fixed. I had no clue you were working live off a PR branch, I would strongly advise against doing that.

@ashjbarnes
Copy link
Collaborator Author

If not, then from a github workflow perspective what's the best way for me to keep working for the moment? I'm losing a lot of time trying to juggle the codebase when this PR is unresolved and I'm already working on the next one (and need it for my day-to-day research). It was becoming too much admin to continually merge changes from this branch into the withtides branch, especially when merge conflicts cropped up.

I then manually made a new branch forked from declutter_notebook to build off but then had to troubleshoot why things weren't working. Perhaps there's a better way of doing things that I'm not aware of?

@ashjbarnes
Copy link
Collaborator Author

Apologies, fixed. I had no clue you were working live off a PR branch, I would strongly advise against doing that.

Yes that makes sense! I wanted to be able to use the improvements from this branch in my day-to-day research, but at some point there were a bunch of annoying merge conflicts when I kept trying to pull changes into withtides. Is it generally best practice to just work on branches from main and wait for PRs to be merged before merging them into your other dev branches, even if it means you're working without some of the new features?

@navidcy
Copy link
Contributor

navidcy commented Dec 13, 2023

I haven't followed any discussion here. @ashjbarnes, @angus-g is this PR under control? If you need help ping me.

…irectories by combining all common files into one folder, and overwriting just the data table and config file as needed. Modify the setup_rundir() method to account for these changes, handling the different use cases by including an override option
@ashjbarnes
Copy link
Collaborator Author

Latest commit implements what we talked about today @angus-g . Notebook has been tested but I'll go back and modify notebooks to add some more documentation - that'll be the last planned commit from me unless something else comes up / breaks!

Thanks @navidcy ! Almost finished :)

…er from my project. Turn off tides. Ensure that toolpath can be either string or path object
…her than dim. Not sure why but new version of glorys does this
regional_mom6/regional_mom6.py Outdated Show resolved Hide resolved
…s both a dimension and a variable. This happens with the new copernicus api downloads
@navidcy
Copy link
Contributor

navidcy commented Jan 16, 2024

tip:
before pushing you should format with black otherwise tests don't run... and tests that fail might hint you about something wrong that is happening. If you postpone formatting black (and thus running the tests) you may not see a bug you introduced until way after you've put substantial effort in refactoring something that you may need to undo.

@ashjbarnes
Copy link
Collaborator Author

Everything tested and working with latest executable. Ready to merge

@ashjbarnes ashjbarnes merged commit d444847 into main Jan 18, 2024
4 of 6 checks passed
@ashjbarnes ashjbarnes deleted the declutter_notebook branch January 18, 2024 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants