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

CLI argument to specify custom output simulation filename suffix #81

Merged
merged 6 commits into from
Mar 3, 2025

Conversation

btobers
Copy link
Collaborator

@btobers btobers commented Mar 3, 2025

Closes #80.

outputfn_sfix argument added to run_simulation.py argparser so that a custom output simulation filename suffix can be passed, which may be useful in some cases. For instance, if particular parameters were being updated for a given glacier and then simulations were run, the default output simulation filename would be used and overwrite previous runs.

@btobers btobers changed the base branch from master to dev March 3, 2025 17:42
@btobers btobers self-assigned this Mar 3, 2025
@btobers
Copy link
Collaborator Author

btobers commented Mar 3, 2025

@ddundo If you have any interest in improving the way simulations are saved, perhaps this would be a good time. I wouldn't be able to work on this for a while, but wanted to mention it to you in case you have the desire to make any improvements on this active PR. I'm sure improvements could be made, but I suspect it'd take quite a bit of work. So unless you have the time to work on doing so, I suggest we close this and further improvements can be made in the future.

One thing I'm not sure about, related to #70, should anything else be included in __all__ for pygem.output?

@btobers btobers marked this pull request as ready for review March 3, 2025 19:01
Copy link
Member

@ddundo ddundo left a comment

Choose a reason for hiding this comment

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

Thanks @btobers. I indeed don't have much time now to dig into this, so I'm happy to merge if you are happy.

There are a few things I noticed that are strange though. For example, I think the use of @dataclass here is not appropriate. But yes, we can get back to this later.

As for __all__, it should include any class or function that we want users to use in their scripts.

@btobers
Copy link
Collaborator Author

btobers commented Mar 3, 2025

Sounds good, thanks @ddundo. I'll close this PR and improvements can be made in the future.

@btobers btobers merged commit a96f3ee into dev Mar 3, 2025
1 check passed
@btobers btobers deleted the 80_cli_outputfn branch March 3, 2025 20:00
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.

Add optional CLI argument for output.outfn in run_simulation.py
2 participants