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

Drop nbscuid name #39

Merged
merged 2 commits into from
Jan 9, 2024
Merged

Drop nbscuid name #39

merged 2 commits into from
Jan 9, 2024

Conversation

mnlevy1981
Copy link
Collaborator

I renamed nbscuid/ -> cupid/, and tried to replace every instance of nbscuid with cupid (including the conda environment names).
I've also updated the README file and verified the instructions provided still work -- this required changing a few paths in the nblibrary YAML file because directories on scratch had been scrubbed.

Also renamed the conda environments, and moved more directory locations from
scratch to campaign
@@ -36,10 +38,10 @@ dependencies = [
]

[project.urls]
repository = "https://github.com/rmshkv/nbscuid"
repository = "https://github.com/NCAR/CUPiD"
documentation = "https://nbscuid.readthedocs.io"
Copy link
Collaborator

@TeaganKing TeaganKing Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be updated in #38 , but seems good to leave as is in this PR.

$ conda env create -f cupid/dev-environment.yml
$ conda activate cupid-dev
$ which cupid-run
$ conda env create -f cupid-analysis.yml
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clear new environment name, thanks for replacing env with analysis, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I figured if we were moving to a single environment for all diagnostics we should have a generic name :) Seeing this block of text, though, I wonder if the file name should be analysis-environment.yml

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like -environment is redundant? And then at least if people are using the environment on a machine with lots of additional environments it may help to know that it's the CUPiD one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment could stay cupid-analysis if the filename changed, and then it would match dev-environment.yml. I'll start a new issue ticket, though, because I think we also want an envs/ directory that contains both environment files rather than having one in cupid/ and the other in the top level

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, seems reasonable to me! Thanks Mike!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thought, actually - I feel like I've seen the standard for environment spec YAML files be to always call the file "environment.yml", and I specifically made the choice to add a descriptor to these ones for clarity, but given that we also have various config YAML files floating around between the different component diagnostics and CUPiD usecases, keeping "environment" might actually be good to lessen confusion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For additional context - the name of the environment is specified in the file and isn't linked to the yml file name)

Copy link
Contributor

@rmshkv rmshkv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, very thorough! :)

@TeaganKing
Copy link
Collaborator

I also double checked all of the nbscuid places that I found against this list of changes, and it looks great to me!

@mnlevy1981 mnlevy1981 merged commit b27ca05 into NCAR:main Jan 9, 2024
@mnlevy1981 mnlevy1981 deleted the drop_nbscuid_name branch January 9, 2024 23:21
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.

3 participants