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.

Sign up for GitHub

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

Jump to bottom

Add fit functions for map computation #872

Merged
merged 17 commits into from
Oct 16, 2024
Merged

Add fit functions for map computation #872

merged 17 commits into from
Oct 16, 2024

Conversation

carhc
Copy link
Collaborator

@carhc carhc commented May 14, 2024

This PR provides the fundamental elements of the map creation, which are the fit functions. It contains the modification of IC fit function, the basic fit functions (and seeds) needed for the map computation and it also defines the current available fits. Other functions to unify input/output are included.

This is done in a different PR to allow for modularity. This functions are useful for #860, #861, #862...

gonzaponte
gonzaponte requested changes May 15, 2024
View reviewed changes
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

First look at this, some structural changes are needed.

invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/core/fit_functions.py Outdated Show resolved Hide resolved
gonzaponte
gonzaponte requested changes Jun 1, 2024
View reviewed changes
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

The problem you have with full_output is due to the scipy version. In IC we have 1.7.1, while you were using some version >1.9.0, which is when the keyword full_output was introduced. I will update the scipy version to 1.9.3 and then you can rebase on top of that.

invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
@gonzaponte gonzaponte mentioned this pull request Jun 1, 2024
Merged
@carhc carhc mentioned this pull request Sep 27, 2024
Draft
carhc added a commit that referenced this pull request Oct 2, 2024
@carhc
878 Update scipy to 1.9.3
adf39c2 
#878

[author: gonzaponte]

This is needed for #872. Tests pass, so we assume the version change
doesn't change anything significant.

[reviewer: carhc]

This new environment, making use of helpful updates in both numpy and
scipy will allow for some new functionalities in IC. Approved!
gonzaponte
gonzaponte reviewed Oct 7, 2024
View reviewed changes
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

This is essentially ready, just a bit of formatting. Please remove unnecessary blank lines at the beginning of functions and so on.

invisible_cities/core/fit_functions_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/core/fit_functions.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
@carhc carhc force-pushed the fit-functions branch 2 times, most recently from 94087c9 to af021e3 Compare October 15, 2024 08:33
gonzaponte
gonzaponte requested changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

Please also add explicitly the equations of the fit functions in the seed-generating functions

invisible_cities/core/fit_functions_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
@carhc carhc force-pushed the fit-functions branch 2 times, most recently from 1c3616e to 869cf55 Compare October 15, 2024 12:34
@gonzaponte gonzaponte requested review from gonzaponte and removed request for jahernando October 15, 2024 13:02
gonzaponte
gonzaponte requested changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

Remember to add the form of the fit function to the docstring in the seed-generating functions.

invisible_cities/reco/icaro_components.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
invisible_cities/reco/icaro_components_test.py Outdated Show resolved Hide resolved
gonzaponte
gonzaponte approved these changes Oct 16, 2024
View reviewed changes
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

First step towards the new map-building procedure. Everything looks ok and is well tested. Good job!

@jwaiton jwaiton merged commit e4b173d into next-exp:master Oct 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gonzaponte gonzaponte gonzaponte approved these changes

Assignees

@carhc carhc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carhc @gonzaponte @jwaiton