maho3 / ltu-ili

Robust ML in Astro
https://ltu-ili.readthedocs.io/en/latest/
46 stars 9 forks source link

119 improve api #123

Closed maho3 closed 10 months ago

maho3 commented 11 months ago

This PR implements numerous structural changes to the configurations of the Data/Inference/Validation stages, without much changes to how they work on the backend. The general theme of this PR is to add various quality-of-life improvements that make it easier to understand the configurations and use ltu-ili for distributed testing. Here's a summary of what has changed.

Removed unnecessary configurations:

Renamed things to make them more obvious:

Conformed the multiple Inference backends to a universal configuration:

Miscellaneous:

Note a lot of files have changed, because we needed to propagate the naming changes to all the examples and tests. I'm happy to walk the reviewer through the listed changes.

codecov-commenter commented 11 months ago

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (a11f4bb) 97.30% compared to head (3dbc6f6) 97.27%.

Files Patch % Lines
ili/dataloaders/loaders.py 79.31% 6 Missing :warning:
ili/inference/runner_sbi.py 96.00% 2 Missing :warning:
ili/inference/runner_pydelfi.py 97.29% 1 Missing :warning:
ili/utils/ndes_pt.py 95.45% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #123 +/- ## ========================================== - Coverage 97.30% 97.27% -0.03% ========================================== Files 18 22 +4 Lines 1113 1249 +136 ========================================== + Hits 1083 1215 +132 - Misses 30 34 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

CompiledAtBirth commented 10 months ago

Currently running tests on a duplicate of this branch (seq_test to use a more recent torch than the setup.cfg).

DeaglanBartlett commented 10 months ago

@CompiledAtBirth how did the tests do on the duplicate branch?

DeaglanBartlett commented 10 months ago

From @CompiledAtBirth on slack:

For the SBISimulator loader specifically, the simulator function has to output a torch.Tensor of torch.Size((1,n)). The case is easy to handle once the user knows his simulator function must return that.

To avoid this mistake, please can you make sure it is well documented in the code and in the tutorial? We might as well do it in this PR as it is a minor thing to add

maho3 commented 10 months ago

From @CompiledAtBirth on slack:

For the SBISimulator loader specifically, the simulator function has to output a torch.Tensor of torch.Size((1,n)). The case is easy to handle once the user knows his simulator function must return that.

To avoid this mistake, please can you make sure it is well documented in the code and in the tutorial? We might as well do it in this PR as it is a minor thing to add

Added comment in the code and tutorial.

maho3 commented 10 months ago

The numpy seed is updated in PlotSinglePosterior if x_obs is None and a seed is provided

This is rather difficult to test because we don't return what x_obs we choose, and the used np seed is difficult to access. I think this test isn't necessary, given the code is pretty clear as to what its doing.

maho3 commented 10 months ago

Check ValidationRunner.load_posterior_sbi() works even if the posterior does not have a name attribute

I just removed this functionality of load_posterior_sbi, because there should never be a case wherein an sbi posterior doesn't have a name. It is given "" by default.