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

Revise circ_diff function #12

Merged
merged 9 commits into from
Jul 9, 2024
Merged

Revise circ_diff function #12

merged 9 commits into from
Jul 9, 2024

Conversation

paulf81
Copy link
Contributor

@paulf81 paulf81 commented Jul 5, 2024

This small pull request makes a small change to the circ_diff function, feel free @aclerc to reject if you prefer as is now, just wanted to offer. Main points:

  1. uses approach similar to the wrap_180 method in FLORIS
  2. Explicitly accepts type list (implied by the tests) (this change could also be made to type hint of original)
  3. No division (avoid risk of division by 0)
  4. I believe it's simpler and should be faster without the trigonometry but yields all passing tests

paulf81 and others added 9 commits July 5, 2024 14:59
@paulf81
Revise circ_diff function
b923d72
@paulf81
fixing type hints for poe
07e9e04
@aclerc
add test_within_bin()
a31e9a5
@paulf81
Merge pull request #1 from aclerc/feature/edit_circ_diff
d74bd5d 
add test_within_bin()
@paulf81
Merge branch 'main' into feature/edit_circ_diff
06395ac
@aclerc
update test_calc_wsratio_v_wd_scen
237370c 
expected result is different due to new circ_diff function
@aclerc
bump version
93c88cb 
increase version because results can be slightly different with new circ_diff method
@aclerc
update smarteole_example.ipynb
62a1622 
ensure outputs are up to date with new circ_diff
minor improvements to plots
@aclerc
Update smarteole_example.ipynb
15ba463
aclerc
aclerc approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@aclerc aclerc left a comment

Choose a reason for hiding this comment

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

Thank you, Paul! The new circ_diff function is a nice improvement. It turns out the previous circ_diff function could be wrong by up to about 1e-12.

@aclerc aclerc merged commit 3ebf2c9 into resgroup:main Jul 9, 2024
1 check passed
@aclerc aclerc deleted the feature/edit_circ_diff branch July 9, 2024 12:04
@paulf81
Copy link
Contributor Author

paulf81 commented Jul 9, 2024

Thank you, Paul! The new circ_diff function is a nice improvement. It turns out the previous circ_diff function could be wrong by up to about 1e-12.

Very glad to hear it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aclerc aclerc aclerc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paulf81 @aclerc