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

Binary derivatives units and exact #1726

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

DotanGazith
Copy link

Fixed the references in ELL1 model (references 1&2 were flipped)
Fixed the units of binary delay derivatives with respect to "angle" parameters (omega\E). Specifically in BT, DD models.
Added an exact derivative class for BT model. I kept it in a separate class to not clash with performance decisions that might drive the approximate derivative implemented.

@DotanGazith
Copy link
Author

Added a fix for the units that caused 3 of the errors.
The black formatting goes back and forth, I format it and the automatic pre-commit un-formats it, not sure what its actual preferences are.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 28.81356% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 68.97%. Comparing base (eb1f48b) to head (8479a2d).
Report is 16 commits behind head on master.

Files Patch % Lines
...c/pint/models/stand_alone_psr_binaries/BT_model.py 26.31% 42 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1726      +/-   ##
==========================================
- Coverage   69.06%   68.97%   -0.09%     
==========================================
  Files         105      105              
  Lines       24700    24768      +68     
  Branches     4409     4414       +5     
==========================================
+ Hits        17058    17084      +26     
- Misses       6534     6576      +42     
  Partials     1108     1108              

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

@dlakaplan
Copy link
Contributor

Hi, @DotanGazith . It looks like the black test is failing just on a single file. I don't know exactly how you have things set up locally but this should be easy to fix in general.

@dlakaplan
Copy link
Contributor

More broadly, do you want to expose your exact BT model to the user? I think you could do that (make a wrapper class in /models/binary_bt.py). You could also add appropriate tests with the same structure as tests that involve BT models, such as tests/test_B1953.py). Finally, it might be good to add some tests to explicitly compare the normal BT against this version (at the moment there shouldn't be any variations since you just copied things, but in case one or the other changes). But some of those additions are up to you.

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