Skip to content

Avoid evolving basis twice#9

Open
comane wants to merge 14 commits intomainfrom
avoid_basis_evolution_issue7
Open

Avoid evolving basis twice#9
comane wants to merge 14 commits intomainfrom
avoid_basis_evolution_issue7

Conversation

@comane
Copy link
Member

@comane comane commented Oct 7, 2024

This pull request includes significant changes to the wmin module, focusing on adding new functionality for weight minimization fits, enhancing configuration parsing, and introducing comprehensive tests. The most important changes include the addition of new modules and methods for exporting fit results, updates to configuration parsing, and the introduction of new test cases.

New Functionality:

  • Export Results Module:
    • Added wmin/export_results.py to include functions for exporting fit results, such as write_wmin_combined_replicas, write_new_lhapdf_info_file_from_previous_pdf, and write_lhapdf_from_ultranest_result.

Configuration Enhancements:

  • Configuration Parsing:
    • Updated wmin/config.py to include logging and error handling for configuration parsing, and added new methods parse_prior_settings and parse_wmin_settings for better handling of prior and wmin settings. [1] [2] [3]

Testing:

  • New Test Cases:
    • Added wmin/tests/test_config.py to include unit tests for the configuration parsing methods, ensuring robust error handling and correct parsing of settings.
    • Added wmin/tests/test_export_results.py to include unit tests for the new export results functions, ensuring they work as expected with mock data.

Miscellaneous:

  • Runcards:
    • Added example runcards for different fit methodologies (wmin_analytic_dis.yaml, wmin_bayes_dis.yaml, wmin_mc_dis.yaml) to demonstrate the usage of the new weight minimization settings. [1] [2] [3]

Minor Changes:

  • Module Import:
    • Updated wmin/app.py to include the new wmin.ultranest_fit module.

Benchmark of the Inherited (wmin) evolution

Wmin Basis used for the test: 241002_NNDF40_basis_pca
Run a fit using wmin_bayes_dis.yaml runcard with wmin_inherited_evolution once true and once false.
The fit with wmin_inherited_evolution: false is the evolved with evolve_fit

https://vp.nnpdf.science/zpmJkg-RTVOwFl4EoKM2xw==

@codecov
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (6054d80) to head (9c12f16).

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #9      +/-   ##
==========================================
+ Coverage   83.63%   90.25%   +6.62%     
==========================================
  Files          12       17       +5     
  Lines         385      616     +231     
==========================================
+ Hits          322      556     +234     
+ Misses         63       60       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@LucaMantani LucaMantani left a comment

Choose a reason for hiding this comment

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

Maybe I am missing something, but I think that it could be done differently so that it applies to both NS and analytic fits. I left a more detailed comment on the ultranest_fit.py file.


wmin_settings = {}

# Set the ultranest seed
Copy link
Member

Choose a reason for hiding this comment

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

The comment is meaningless, probably result of copy/paste

Copy link
Member

Choose a reason for hiding this comment

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

I am not a huge fan of this override, also at the moment I think you are losing the writing of the bayes results.

I think a better strategy could be to override write_replicas and have that it does the evolution as well instead of simply writing the export grids. All the missing info (wmin_settings, errortype etc) can be put in the pdf_model instance (and I think they are already there most of them).

One advantage of doing it this way is that it would apply to both analytic and ultranest fit, without touching them.

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