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

Added possibility to save results #148

Merged
merged 18 commits into from
May 19, 2021
Merged

Added possibility to save results #148

merged 18 commits into from
May 19, 2021

Conversation

thuiop
Copy link
Collaborator

@thuiop thuiop commented May 3, 2021

Closes #142

I still have to add utility functions for loading the saved results easily.

@thuiop thuiop added the feature add new feature label May 3, 2021
@thuiop thuiop self-assigned this May 3, 2021
@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #148 (489af69) into main (d198d09) will increase coverage by 0.82%.
The diff coverage is 93.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   80.33%   81.15%   +0.82%     
==========================================
  Files          10       11       +1     
  Lines        1144     1215      +71     
==========================================
+ Hits          919      986      +67     
- Misses        225      229       +4     
Flag Coverage Δ
unittests 81.15% <93.05%> (+0.82%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
btk/metrics.py 94.23% <87.50%> (+0.15%) ⬆️
btk/utils.py 90.69% <90.69%> (ø)
btk/__init__.py 100.00% <100.00%> (ø)
btk/draw_blends.py 85.58% <100.00%> (+0.58%) ⬆️
btk/measure.py 77.67% <100.00%> (+2.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d198d09...489af69. Read the comment docs.

@thuiop thuiop requested a review from aboucaud May 5, 2021 10:27
@thuiop
Copy link
Collaborator Author

thuiop commented May 5, 2021

This PR is ready to be reviewed ; the only shortcoming here is that PSF and WCS are not saved with the blends as galsim and astropy do not provide utility functions to do it cleanly. Should we use something like pickle to save it anyway ?

@thuiop thuiop added the Ready for review For PR which should be reviewed label May 5, 2021
Copy link
Collaborator

@ismael-mendoza ismael-mendoza left a comment

Choose a reason for hiding this comment

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

Looks very good @thuiop, thanks for adding this functionality. Please see below for a couple of small requested changes and questions. It appears to me that it's really close to being ready for merge

btk/draw_blends.py Outdated Show resolved Hide resolved
btk/measure.py Outdated Show resolved Hide resolved
btk/measure.py Outdated Show resolved Hide resolved
btk/measure.py Outdated Show resolved Hide resolved
btk/utils.py Show resolved Hide resolved
tests/test_save.py Outdated Show resolved Hide resolved
btk/utils.py Outdated Show resolved Hide resolved
btk/utils.py Outdated Show resolved Hide resolved
btk/utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@aboucaud aboucaud left a comment

Choose a reason for hiding this comment

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

I agree with @ismael-mendoza, one should be more attentive to how the save_path is constructed.

The way you wrote it, the user does not really have a choice for the filenames (probably a good thing), but it must have a choice of the main output directory then. If this is what you mean by save_path then we should use pathlib to construct the full paths.
Also it may be easier (we can discuss this) to create subdirectories inside the save_path to put the various outputs, e.g. save_path/survey/blended.npy rather than <save_path>_<survey>_blended.npy. That way it is easier to check the directory existence rather than the existence of each file before loading.

@thuiop
Copy link
Collaborator Author

thuiop commented May 18, 2021

I should have addressed all the comments.

ismael-mendoza
ismael-mendoza previously approved these changes May 18, 2021
Copy link
Collaborator

@ismael-mendoza ismael-mendoza left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge after the CI passes and if my commit suggestions below are indeed what you intended.

btk/draw_blends.py Show resolved Hide resolved
btk/draw_blends.py Outdated Show resolved Hide resolved
btk/measure.py Outdated Show resolved Hide resolved
btk/measure.py Outdated Show resolved Hide resolved
btk/metrics.py Outdated Show resolved Hide resolved
btk/metrics.py Outdated Show resolved Hide resolved
Co-authored-by: Ismael Mendoza <[email protected]>
thuiop and others added 3 commits May 19, 2021 00:54
Co-authored-by: Ismael Mendoza <[email protected]>
Co-authored-by: Ismael Mendoza <[email protected]>
Co-authored-by: Ismael Mendoza <[email protected]>
@ismael-mendoza
Copy link
Collaborator

Thanks for resolving the merge conflicts @thuiop. Merging now -

@ismael-mendoza ismael-mendoza merged commit 2c5e258 into main May 19, 2021
@ismael-mendoza ismael-mendoza deleted the save-results branch May 19, 2021 12:09
@ismael-mendoza ismael-mendoza mentioned this pull request Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature add new feature Ready for review For PR which should be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a functionnality to export the measurement results
3 participants