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

Feature/order samples unsequa #766

Merged
merged 17 commits into from
Aug 24, 2023

Conversation

chahank
Copy link
Member

@chahank chahank commented Aug 4, 2023

Changes proposed in this PR:

  • Add two examples to the tutorials on how to use the unseques modules for coupled input variables and for handling efficiently the loading of multiple large files
  • Adds a method to order the samples in unsequa

Note: this pull requests uses the loglevel from #765 . All changes not in the tutorial file are due to the other pull requests. Please merge #765 first.

PR Author Checklist

PR Reviewer Checklist

@chahank chahank requested a review from peanutfun August 4, 2023 14:42
@chahank
Copy link
Member Author

chahank commented Aug 10, 2023

Note: tests for petals fail due to the landslide module. This is unrelated to this PR....

@chahank chahank requested a review from simonameiler August 11, 2023 09:40
@chahank chahank requested review from timschmi95 and removed request for peanutfun August 16, 2023 15:06
@timschmi95
Copy link
Collaborator

@chahank Where in the notebook are the added examples? The table of contents is the same between the two versions.
If I know which examples, I can review this added part although I've never used the unsequa module. Someone who has used it may be able to provide better feedback regarding how the added parts tie in with the unseque module in general.

@chahank
Copy link
Member Author

chahank commented Aug 17, 2023

It is all the "advanced examples" at the bottom. I do not know why the sections do not show up in the t.o.c., I will have to fix this. Thank!

Copy link
Collaborator

@timschmi95 timschmi95 left a comment

Choose a reason for hiding this comment

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

Without having used the unsequa module I think the the point comes across how one can (1) share an input parameter among multiple input variables and (2) using a global variable to avoid reloading files.
But a second reviewers opinion, who has used the unsequa module, is needed as I didn't follow the details in the code.
Regarding layout, if there is an option the not print the details (e.g. Cost Benefit table) for each variable combination it would improve the readability of the tutorial.
Lastly, would pulling the latest changes from the target branch feature/unsequa_multiprocessing fix the failing tests as they seem to pass in PR #763 ?

@@ -112,6 +112,22 @@ def __init__(self, samples_df, unit=None):
self.samples_df = samples_df
self.unit = unit

def order_samples(self, by):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docstring has the argument name 'by_unc_params' instead of 'by'.

@chahank
Copy link
Member Author

chahank commented Aug 22, 2023

Thanks!

For the printing from cost benefit, this is unfortunately backed in the module at the moment. But since this will be rewritten in the coming year, we should be able to solve this. For the tutorial I can simply remove it.

And the failing test is puzzling, as it is completely unrelated. I will see to fix it.

@chahank chahank changed the base branch from feature/unsequa_multiprocessing to develop August 23, 2023 09:42
@chahank chahank changed the base branch from develop to feature/unsequa_multiprocessing August 23, 2023 09:43
@chahank chahank removed the request for review from simonameiler August 24, 2023 12:24
@chahank chahank merged commit a21abe3 into feature/unsequa_multiprocessing Aug 24, 2023
peanutfun added a commit that referenced this pull request Aug 28, 2023
* Update pandas iteritems to items
* Change pd append to concat
* Fix minimal chunksize at 1
* Remove not needed parallel pool chunksize argument
* Remove global matplotlib styles
* Remove deprecated tot_value from unsequa
* Add chunked version of parallel computing
* Remove deprecated numpy vstack of objects
* Feature/order samples unsequa (#766)
* Allow to set loglevel
* Add method to sort samples
* Add advanced examples for unsequa
* Remove logging control
* Remove unecessary output prints
* Update CHANGELOG.md

---------

Co-authored-by: Chahan Kropf <chahan.kropf@posteo.com>
Co-authored-by: emanuel-schmid <schmide@ethz.ch>
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
@emanuel-schmid emanuel-schmid deleted the feature/order_samples_unsequa branch September 7, 2023 13:27
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