Skip to content

(#80) node to element interpolation fix#98

Merged
armantekinalp merged 6 commits into
GazzolaLab:update-0.3.0from
bhosale2:80_node_to_element_vel_fix
Jun 1, 2022
Merged

(#80) node to element interpolation fix#98
armantekinalp merged 6 commits into
GazzolaLab:update-0.3.0from
bhosale2:80_node_to_element_vel_fix

Conversation

@bhosale2
Copy link
Copy Markdown
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 requested a review from armantekinalp June 1, 2022 04:26
@bhosale2 bhosale2 linked an issue Jun 1, 2022 that may be closed by this pull request
Converting nodal velocities to element velocities #80
Closed
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 1, 2022 •
edited
Loading

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
Copy Markdown
Collaborator

skim0119 commented Jun 1, 2022 •
edited
Loading

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

@bhosale2
Copy link
Copy Markdown
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
Copy Markdown
Contributor

armantekinalp commented Jun 1, 2022

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

@bhosale2
Copy link
Copy Markdown
Collaborator Author

bhosale2 commented Jun 1, 2022

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

@skim0119
Copy link
Copy Markdown
Collaborator

skim0119 commented Jun 1, 2022

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

@bhosale2
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

armantekinalp commented Jun 1, 2022

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"}

armantekinalp
armantekinalp approved these changes Jun 1, 2022
View reviewed changes
Copy link
Copy Markdown
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
@bhosale2 bhosale2 mentioned this pull request Jun 1, 2022
Closed
pppprub pushed a commit to pppprub/PyElastica that referenced this pull request Sep 4, 2022
@armantekinalp
Merge pull request GazzolaLab#98 from armantekinalp/97_warning_mass_m…
1fd6ea8 
…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

Reviewers

@armantekinalp armantekinalp armantekinalp approved these changes

Assignees

@bhosale2 bhosale2

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Milestone

Version 0.3

Development

Successfully merging this pull request may close these issues.

Converting nodal velocities to element velocities

4 participants

@bhosale2 @codecov-commenter @skim0119 @armantekinalp