Skip to content

Adapt for online lab compatibility#23

Merged
juntyr merged 6 commits into
mainfrom
online-lab
Apr 15, 2025
Merged

Adapt for online lab compatibility#23
juntyr merged 6 commits into
mainfrom
online-lab

Conversation

@juntyr
Copy link
Copy Markdown
Collaborator

@juntyr juntyr commented Apr 15, 2025

With these changes, the following top-to-bottom example works: https://gist.github.com/juntyr/ea85adab5df19141a84939f2d322697f

Comment thread pyproject.toml
Comment thread pyproject.toml
@@ -1,4 +1,4 @@
__all__ = ["compressors", "metrics", "tests"]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm definitely amenable to a different name, this just makes things work with the least effort without making anything worse

Comment thread src/climatebenchpress/compressor/scripts/collect_metrics.py
Comment thread src/climatebenchpress/compressor/scripts/collect_metrics.py
Comment thread src/climatebenchpress/compressor/scripts/compress.py
@juntyr juntyr requested a review from treigerm April 15, 2025 08:03
Copy link
Copy Markdown
Member

@treigerm treigerm left a comment

Choose a reason for hiding this comment

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

Thanks @juntyr ! Looks good to me just had some very minor questions, feel free to merge when you want to.

Overall, the structure of having the scripts as modules of the libraries seems a bit unusual (in the sense that I don't think I have seen Python libaries do this before). But it should be fine for the EGU demo. Long term we should probably think about how we can refactor the scripts into some re-usable library components.

Comment thread src/climatebenchpress/compressor/scripts/__init__.py
Comment thread src/climatebenchpress/compressor/scripts/collect_metrics.py
Comment thread src/climatebenchpress/compressor/scripts/collect_metrics.py
@juntyr
Copy link
Copy Markdown
Collaborator Author

juntyr commented Apr 15, 2025

Overall, the structure of having the scripts as modules of the libraries seems a bit unusual (in the sense that I don't think I have seen Python libaries do this before). But it should be fine for the EGU demo. Long term we should probably think about how we can refactor the scripts into some re-usable library components.

What if we call the module something other than scripts ...

On a more serious note, I feel like the compression and metric modules would not change much under a different structure (and we already use the executable module approach in the data loader). The error bound generation and summary printing are definitely more in script land.

For this PR, I just wanted to ensure that code importing the library can reuse all of the functionality. I agree that we can probably find a better way to organize them. If you can think of something, I can make the changes now, otherwise we can come back to it later

@treigerm
Copy link
Copy Markdown
Member

No feel free to merge, I was mainly just thinking out loud. Feel free to merge!

@juntyr juntyr merged commit cb2d35b into main Apr 15, 2025
3 checks passed
@juntyr juntyr deleted the online-lab branch April 15, 2025 11: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.

2 participants