-
Notifications
You must be signed in to change notification settings - Fork 8
tidying MAN #497
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
Open
CB-quakemodel
wants to merge
67
commits into
master
Choose a base branch
from
clean_man
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
tidying MAN #497
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kejohnso
reviewed
Oct 8, 2025
kejohnso
reviewed
Oct 8, 2025
| @@ -1,42 +1,40 @@ | |||
| # ------------------- The OpenQuake Model Building Toolkit -------------------- | |||
Collaborator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to think about this change.... I don't think I'm exaggerating when I say it would break 100s of my scripts but I do understand the point changing the submodule organization
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR cleans the MAN module in the following ways:
All python modules have been cleaned (removing unused imports, ordering imports, adding the GEM copyright to top of each file, consistent formatting and for functions with many inputs making them one input per line to make less cluttered)
Moving similar sets of functions into the same python modules. For example, the functions pertaining to mfds and rates have all been moved into a single module, rather than being dispersed as before.
Cleaning up the tests - the data for each test is now separate and clearly labelled, as before they were in mixed directories).
Cleaning the notebooks to be more readily usable and updating the imports to match the new MAN module structure.
I remove the
csv_sitefunction as it is very arbitrary (just prints a site amp function), and also the unit tests are incomplete (they are just a string which I guess was going to be the expected results of using this function, but I see no test data or unit test classes here either).Better defining each set of functions. For example the functions which work on a single source typology are now stored in a folder called
single_source_utilsrather than justsinglewhich was a bit ambiguous. Another example is thechecksdirectory, which is now calledchecking_utilsand has more clearly defined python modules within it - the MFD and rates functions 2 separate modules both calledmfd.py, and a module calledrates.pyand have been merged into a single python module given they were heavily interlinked (the new module is calledmfds_and_rates.py)