Skip to content

Conversation

@timmens
Copy link
Member

@timmens timmens commented Apr 7, 2023

Changes

  • Move tranquilo folder directly under src
  • Move subsolver folder in tranquilo
  • Do the same for tests
  • Remove all other estimagic files
  • Adjust import paths
  • Add new files and copy contents:
    • exploration_sample.py from tiktok.py
    • utilities.py from utilities.py
  • Update configuration files, environment files, etc.
  • Adjust tests to use functions from tranquilo not estimagic. Example
    • Before:
    minimize(..., algorithm="tranquilo")
    • Now:
    from tranquilo.tranquilo import tranquilo
    minimize(..., algorithm=tranquilo)
  • Delete CITATION
  • Update LICENSE
  • Update CHANGES.md
  • Put tranquilo in __init__.py
  • Move wrapped_subsolver.py to subsolvers
  • Adjust codecov

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #2 (3dc4d6d) into main (f66089d) will decrease coverage by 15.20%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main       #2       +/-   ##
===========================================
- Coverage   93.24%   78.05%   -15.20%     
===========================================
  Files         247       68      -179     
  Lines       17987     4666    -13321     
===========================================
- Hits        16772     3642    -13130     
+ Misses       1215     1024      -191     
Impacted Files Coverage Δ
src/tranquilo/__init__.py 100.00% <ø> (ø)
src/tranquilo/acceptance_decision.py 96.87% <ø> (ø)
src/tranquilo/acceptance_sample_size.py 95.45% <ø> (ø)
src/tranquilo/adjust_radius.py 100.00% <ø> (ø)
src/tranquilo/aggregate_models.py 97.67% <ø> (ø)
src/tranquilo/bounds.py 94.11% <ø> (ø)
src/tranquilo/clustering.py 23.80% <ø> (ø)
src/tranquilo/config.py 100.00% <ø> (ø)
src/tranquilo/estimate_variance.py 100.00% <ø> (ø)
src/tranquilo/exploration_sample.py 90.32% <ø> (ø)
... and 31 more

... and 127 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@janosg janosg left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I went over all the imports and a couple of places where I thought you might miss something. Looks very good! I would suggest a few more infrastructure simplifications:

  • The rtd_environment has some redundant packages and is missing estimagic
  • The folder docs/source/scripts can be deleted
  • MANIFEST.in mentions .db files. I think we can remove that
  • pytask and click can be removed from environment.yml
  • pytask related options can be removed from pyproject.toml
  • Some of the ignored warnings in pyproject.toml can be removed

I would suggest we merge quickly and get the new package up on PyPi and conda-forge. We don't have users yet and can always fix a few more rough edges later.

@timmens timmens merged commit ad05f88 into main Apr 9, 2023
@timmens timmens deleted the pruning branch April 9, 2023 17:34
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