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

dlogX -> logdX #306

Merged
merged 4 commits into from
Jun 29, 2023
Merged

dlogX -> logdX #306

merged 4 commits into from
Jun 29, 2023

Conversation

williamjameshandley
Copy link
Collaborator

Description

This has come up in the past, but explicitly today with margret, the notation dlogX is unfortunate, in that it is quite definitely wrong. In the spirit of logL, it should really be logdX since it is the log of small volume shell dX. This makes expressions make a little more sense.

This is a breaking change (albeit a minor one -- very few users will be using this underlying code), so it should be merged before 2.0.0 release. I have added a helpful error to the old notation.

Checklist:

  • I have performed a self-review of my own code
  • My code is PEP8 compliant (flake8 anesthetic tests)
  • My code contains compliant docstrings (pydocstyle --convention=numpy anesthetic)
  • New and existing unit tests pass locally with my changes (python -m pytest)
  • I have added tests that prove my fix is effective or that my feature works
  • I have appropriately incremented the semantic version number in both README.rst and anesthetic/_version.py

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (98a4eb2) 100.00% compared to head (d2e12f2) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #306   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           30        30           
  Lines         2665      2667    +2     
=========================================
+ Hits          2665      2667    +2     
Impacted Files Coverage Δ
anesthetic/_version.py 100.00% <100.00%> (ø)
anesthetic/samples.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@lukashergt lukashergt left a comment

Choose a reason for hiding this comment

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

This definitely clears up the notation, very nice. Should we add a check for logdX's existence?

tests/test_samples.py Show resolved Hide resolved
Copy link
Collaborator

@lukashergt lukashergt left a comment

Choose a reason for hiding this comment

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

Great, please squash and merge.

@williamjameshandley williamjameshandley merged commit a5d4620 into master Jun 29, 2023
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