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

(#80) node to element interpolation fix #98

Merged

Conversation

bhosale2
Copy link
Collaborator

@bhosale2 bhosale2 commented Jun 1, 2022

Fixes #80, by

  1. Adding momentum conserving node-to-element interpolation for velocity.
  2. Renaming older node-to-element interpolation as to be used for position.
  3. Specifically naming the node-to-element interpolation used for mass/forces (edge special treatment).

@armantekinalp I ran the snake and flagella, and they produce results as expected.

@bhosale2 bhosale2 added bug Something isn't working enhancement New feature or request labels Jun 1, 2022
@bhosale2 bhosale2 added this to the Version 0.3 milestone Jun 1, 2022
@bhosale2 bhosale2 self-assigned this Jun 1, 2022
@bhosale2 bhosale2 linked an issue Jun 1, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2022

Codecov Report

❗ No coverage uploaded for pull request base (update-0.3.0@cbc24d9). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             update-0.3.0      #98   +/-   ##
===============================================
  Coverage                ?   87.13%           
===============================================
  Files                   ?       41           
  Lines                   ?     2550           
  Branches                ?      347           
===============================================
  Hits                    ?     2222           
  Misses                  ?      306           
  Partials                ?       22           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbc24d9...376ece2. Read the comment docs.

@skim0119
Copy link
Collaborator

skim0119 commented Jun 1, 2022

@bhosale2 Instead of renaming, can you make an alias and leave a deprecation warning?

@bhosale2
Copy link
Collaborator Author

bhosale2 commented Jun 1, 2022

@skim0119 per discussion with @armantekinalp in aacc827:

  1. For the pure renaming, as per your suggestion I have added an alias with a deprecation warning.
  2. For the velocity interpolation, the function signature and computation itself change, so to avoid the users using the incorrect version, we now raise a NotImplementedError in the old version, with reference to the correct versions in the error message.

@armantekinalp
Copy link
Contributor

Should we add tests for depreciated functions to keep coverage at the same level. What do you think @skim0119 , @bhosale2

@bhosale2
Copy link
Collaborator Author

bhosale2 commented Jun 1, 2022

@armantekinalp we can use pytest.raises and pytest.warns to test those functions.

@skim0119
Copy link
Collaborator

skim0119 commented Jun 1, 2022

I'm adding little more info for the deprecation note.

@bhosale2
Copy link
Collaborator Author

bhosale2 commented Jun 1, 2022

@armantekinalp and @skim0119 tests for the deprecated/not impl. versions have been added in 376ece2.

@skim0119
Copy link
Collaborator

skim0119 commented Jun 1, 2022

@armantekinalp
Coverage is not working. Can you double check the token?

Codecov report uploader 0.2.3
[20[22](https://github.com/GazzolaLab/PyElastica/runs/6693471477?check_suite_focus=true#step:13:23)-06-01T16:20:59.822Z] ['info'] => Project root located at: /home/runner/work/PyElastica/PyElastica
[2022-06-01T16:20:59.8[23](https://github.com/GazzolaLab/PyElastica/runs/6693471477?check_suite_focus=true#step:13:24)Z] ['info'] -> No token specified or token is empty
[2022-06-01T16:20:59.973Z] ['info'] Searching for coverage files...
[2022-06-01T16:20:59.998Z] ['info'] Warning: Some files located via search were excluded from upload.
[2022-06-01T16:20:59.998Z] ['info'] If Codecov did not locate your files, please review https://docs.codecov.com/docs/supported-report-formats
[2022-06-01T16:20:59.998Z] ['error'] There was an error running the uploader: No coverage files located, please try use `-f`, or change the project root with `-R`

@armantekinalp
Copy link
Contributor

It is working now, I updated the secret token

Codecov report uploader 0.2.3
[23](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:24)
[2022-06-01T19:50:23.533Z] ['info'] => Project root located at: /home/runner/work/PyElastica/PyElastica
[24](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:25)
[2022-06-01T19:50:23.535Z] ['info'] -> No token specified or token is empty
[25](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:26)
[2022-06-01T19:50:23.747Z] ['info'] Running coverage xml...
[26](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:27)
[2022-06-01T19:50:24.794Z] ['info'] Searching for coverage files...
[27](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:28)
[2022-06-01T19:50:24.865Z] ['info'] Warning: Some files located via search were excluded from upload.
[28](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:29)
[2022-06-01T19:50:24.865Z] ['info'] If Codecov did not locate your files, please review https://docs.codecov.com/docs/supported-report-formats
[29](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:30)
[2022-06-01T19:50:24.865Z] ['info'] => Found 1 possible coverage files:
[30](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:31)
  coverage.xml
[31](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:32)
[2022-06-01T19:50:24.865Z] ['info'] Processing /home/runner/work/PyElastica/PyElastica/coverage.xml...
[32](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:33)
[2022-06-01T19:50:24.872Z] ['info'] Detected GitHub Actions as the CI provider.
[33](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:34)
[2022-06-01T19:50:24.875Z] ['info'] Pinging Codecov: https://codecov.io/upload/v4?package=github-action-3.1.0-uploader-0.2.3&token=*******&branch=80_node_to_element_vel_fix&build=2422940027&build_url=https%3A%2F%2Fgithub.com%2FGazzolaLab%2FPyElastica%2Factions%2Fruns%2F2422940027&commit=376ece21b204b4382f31b352f6d992878b28da02&job=CI&pr=98&service=github-actions&slug=GazzolaLab%2FPyElastica&name=&tag=&flags=&parent=
[34](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:35)
[2022-06-01T19:50:25.491Z] ['info'] https://codecov.io/github/GazzolaLab/PyElastica/commit/376ece21b204b4382f31b352f6d992878b28da02
[35](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:36)
https://storage.googleapis.com/codecov/v4/raw/2022-06-01/0EAE7D8E6F10801B0C4E3F2654FD1936/376ece21b204b4382f31b352f6d992878b28da02/7ac7138e-0980-41b1-9383-382c82fc4976.txt?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=GOOG1EKKHVKCKHW7KBCGM7IHR55T63V2PAVJWLVFNITJHDU5G6R5IRN3LMWJA%2F20220601%2FUS%2Fs3%2Faws4_request&X-Amz-Date=20220601T195025Z&X-Amz-Expires=10&X-Amz-SignedHeaders=host&X-Amz-Signature=70370a31d78864457ed29b92280e54da7638f6b14bbb5c235d47f1e9431043de
[36](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:37)
[2022-06-01T19:50:25.492Z] ['info'] Uploading...
[37](https://github.com/GazzolaLab/PyElastica/runs/6696748680?check_suite_focus=true#step:13:38)
[2022-06-01T19:50:25.680Z] ['info'] {"status":"success","resultURL":"https://codecov.io/github/GazzolaLab/PyElastica/commit/376ece21b204b4382f31b352f6d992878b28da02"}

Copy link
Contributor

@armantekinalp armantekinalp left a comment

Choose a reason for hiding this comment

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

LGTM

@armantekinalp armantekinalp merged commit 9e461aa into GazzolaLab:update-0.3.0 Jun 1, 2022
@bhosale2 bhosale2 deleted the 80_node_to_element_vel_fix branch June 1, 2022 22:19
pppprub pushed a commit to pppprub/PyElastica that referenced this pull request Sep 4, 2022
…oment_inertia

update: warning for too small mass moment inertia
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Converting nodal velocities to element velocities
4 participants