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

Setup #1

Merged
merged 72 commits into from
Nov 27, 2023
Merged

Setup #1

merged 72 commits into from
Nov 27, 2023

Conversation

znichollscr
Copy link
Contributor

@znichollscr znichollscr commented Nov 2, 2023

Sets up the repository and adds a basic pipeline that can be used for testing, refactoring etc.

Copy link
Contributor

@lewisjared lewisjared left a comment

Choose a reason for hiding this comment

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

Awesome stuff. This is looking very professional. pydoit-nb is starting to get conceptually busy so clarifying some of the terms that we use will be pretty important soon. I still don't have a clear definition of Branch in my head (is it simply a collection of notebooks or is it more in line with running multiple scenarios?).

I like the splitting of tasks and config

I could checkout and run successfully

– put notebook number at start of saved data file, makes it easier to find where it came from later
– make clean data and plots in same notebook first (fast iteration), but make sure you go back and split in follow up step (fast iteration for plots)
- config gets hydrated and written to disk so put sensitive information in environment variables
- config class plus cattrs is way of getting around awkwardness of passing things in memory (pass everything via serialising to disk which makes things a bit slower and more IO intense, but also way simpler, put note that if your setup does heaps of IO for the config, this solution may not work for you/you may need to work out how to pull things out of the config into somewhere else)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be super surprised if this was a limitation unless you were running each magicc instance as a separate notebook step. Have this been an issue in the past?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's helpful to have in your head so you realise that you probably don't want everything in the config object. In the carbon budget crunching stuff, I was just serialising ScmData objects to yaml but I don't think that's very smart. I think having a note somewhere that the config class gets serialised as yaml, and that might not always be the smartest choice (e.g. it's often better to serialise ScmData to netCDF and then just pass the name of the config file around via yaml rather than the full data itself), can be helpful.

dodo.py Show resolved Hide resolved
dodo.py Outdated Show resolved Hide resolved
dodo.py Outdated Show resolved Hide resolved
notebooks/0xx_preparation/000_write-seed.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
scripts/update-dev-config.py Show resolved Hide resolved
src/local/config/__init__.py Show resolved Hide resolved
src/local/pydoit_nb/tasks_notebooks.py Outdated Show resolved Hide resolved
src/local/pydoit_nb/tasks_notebooks.py Outdated Show resolved Hide resolved
@znichollscr znichollscr requested a review from mikapfl November 22, 2023 08:26
@znichollscr
Copy link
Contributor Author

@JGuetschow tagging you here, I can add you as a reviewer once you join the CR GitHub group

@znichollscr
Copy link
Contributor Author

@lewisjared @JGuetschow @mikapfl last chance for any comments or follow up here. Otherwise I'll merge Monday and we can deal with issues/refactorings in a more narrow context.

@znichollscr znichollscr changed the title Draft: Setup Setup Nov 24, 2023
@mikapfl
Copy link

mikapfl commented Nov 24, 2023

@lewisjared @JGuetschow @mikapfl last chance for any comments or follow up here. Otherwise I'll merge Monday and we can deal with issues/refactorings in a more narrow context.

I think you showing it and our discussion based on it was super useful for me. This PR here is a lot of code, so I didn't actually look into it. Any more detailed feedback from me would probably happen after I worked with this setup.

@znichollscr
Copy link
Contributor Author

I think you showing it and our discussion based on it was super useful for me. This PR here is a lot of code, so I didn't actually look into it. Any more detailed feedback from me would probably happen after I worked with this setup.

Very fair. I hope to get you working with it in the week of December 5 just as fyi (only checking things run though, not actually doing development)

@znichollscr znichollscr merged commit 38160e7 into main Nov 27, 2023
5 checks passed
@znichollscr znichollscr deleted the setup branch January 2, 2024 18:11
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