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

Some cleanup #34

Merged
merged 11 commits into from
Jun 15, 2023
Merged

Some cleanup #34

merged 11 commits into from
Jun 15, 2023

Conversation

navidcy
Copy link
Contributor

@navidcy navidcy commented Jun 14, 2023

Just some organizing.

@ashjbarnes, what does regional_model_scripts.py includes? What's the difference from the mom6-regiona.py?

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #34 (231f6aa) into main (f7ad32b) will not change coverage.
The diff coverage is n/a.

❗ Current head 231f6aa differs from pull request most recent head f514782. Consider uploading reports for the commit f514782 to get more accurate results

@@           Coverage Diff           @@
##             main      #34   +/-   ##
=======================================
  Coverage   10.72%   10.72%           
=======================================
  Files           2        2           
  Lines         429      429           
=======================================
  Hits           46       46           
  Misses        383      383           

@navidcy navidcy changed the title Move regional_model_scripts.py into mom6-regional directory` Move regional_model_scripts.py into mom6-regional directory + add Black badge Jun 14, 2023
@angus-g
Copy link
Collaborator

angus-g commented Jun 14, 2023

I'm not sure it makes sense to move that into the library. Probably better to just delete it, we can excavate it from history if ever needed.

@navidcy
Copy link
Contributor Author

navidcy commented Jun 14, 2023

Oh, you said the magic word!

@navidcy navidcy changed the title Move regional_model_scripts.py into mom6-regional directory + add Black badge Add Black badge + some cleanup Jun 14, 2023
@angus-g
Copy link
Collaborator

angus-g commented Jun 14, 2023

If @ashjbarnes thinks we should keep it, I'd vote for rewriting it as a command-line tool of some kind (still in Python). It would be distributed with the package, but not used as a script. But my understanding was that the library is meant to supersede it all!

@ashjbarnes
Copy link
Collaborator

The only reason the legacy mom6_regional_scripts.py is still there is I've not incorporated the Regrid Runoff step. Perhaps we could delete it for now, and then alter the data table in the default rundirs so they're not expecting runoff at all? Make a note that runoff will / might be included at some point

@navidcy
Copy link
Contributor Author

navidcy commented Jun 14, 2023

@ashjbarnes, feel free to alter the data table in the default run directory in this PR?

@navidcy
Copy link
Contributor Author

navidcy commented Jun 14, 2023

@ashjbarnes shall I merge and you deal w runoff in a different PR?

@ashjbarnes
Copy link
Collaborator

Oh good point - I think we should deal with it now because otherwise the demo notebooks won't work after merging.

How do I modify within the PR? Go back to my review and change some code?

@navidcy
Copy link
Contributor Author

navidcy commented Jun 15, 2023

locally checkout the branch and then commit to it!

git checkout ncc/move-regional-model-scripts

Makes sense?

@ashjbarnes
Copy link
Collaborator

Ok I'm going to draw this one out a bit to make sure that on merging the demo notebooks work. I'll remove the runoff from the runs, modify notebook accordingly, and ensure that the demo still produces a runnable mom6 configuration

@navidcy
Copy link
Contributor Author

navidcy commented Jun 15, 2023

Yeah, do that!

@navidcy navidcy changed the title Add Black badge + some cleanup Some cleanup Jun 15, 2023
@navidcy
Copy link
Contributor Author

navidcy commented Jun 15, 2023

I moved the black badge to a different PR; see #36

@navidcy
Copy link
Contributor Author

navidcy commented Jun 15, 2023

@ashjbarnes I'll let you merge this PR as soon as you have tested that the demo notebook run or whenever you are happy.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ashjbarnes
Copy link
Collaborator

Thanks Navid! I'm happy with this now. The demo notebooks could do with some more (or better thought out) markdown but they work fine for now. I'll save this for a future PR

@ashjbarnes ashjbarnes merged commit e3e59a6 into main Jun 15, 2023
@ashjbarnes ashjbarnes deleted the ncc/move-regional-model-scripts branch June 15, 2023 07:39
@ashjbarnes ashjbarnes restored the ncc/move-regional-model-scripts branch June 15, 2023 07:39
@ashjbarnes ashjbarnes deleted the ncc/move-regional-model-scripts branch June 15, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants