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

sign convention docs and proof #248

Merged
merged 13 commits into from
May 2, 2024
Merged

sign convention docs and proof #248

merged 13 commits into from
May 2, 2024

Conversation

bradcarman
Copy link
Contributor

Adding documentation about the applied sign convention. No changes were made to the current library except to add Constant input sources which aligned with the already available input sources. This PR replaces the previous incorrect PR #246.

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

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

Project coverage is 54.28%. Comparing base (e6ffefa) to head (fba3324).

Files Patch % Lines
src/Hydraulic/IsothermalCompressible/components.jl 15.78% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            main     #248       +/-   ##
==========================================
+ Coverage   0.00%   54.28%   +54.28%     
==========================================
  Files         33       46       +13     
  Lines       1630     1667       +37     
==========================================
+ Hits           0      905      +905     
+ Misses      1630      762      -868     

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

@baggepinnen
Copy link
Contributor

something appears off with this PR, it looks like every single file has been modified by the formatter?

@bradcarman
Copy link
Contributor Author

I truly modified about 15 files. Formatter, maybe a new update, modified a few others.

@baggepinnen
Copy link
Contributor

baggepinnen commented Jan 9, 2024

This PR introduces ConstantForce, ConstantCurrent and ConstantMassFlow which are against our convention of having a single Force component that takes a signal (Constant in this case) as input. The reason for splitting the shape of the input out like this is that otherwise we'd need a ton of components,

  • ConstantForce
  • SineForce
  • SquareWaveForce
  • ...
  • ConstantVoltage
  • SineVoltage
  • ...
  • ConstantMassFlow
  • ...

Exploding into n_domains x n_shapes components.

For tutorials in particular, if a new users sees
image
they immediately understand how to make the force sinusoidal instead of constant, but if they just see a ConstantForce block, they will be confused when they do not see a SineForce block

@bradcarman
Copy link
Contributor Author

Ah OK, makes sense. I removed the ConstantX components and define them simply in the "sign_convention.md" doc for illustration purposes only.

Copy link
Member

@ven-k ven-k left a comment

Choose a reason for hiding this comment

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

This PR looks great!

docs/src/connectors/sign_convention.md Outdated Show resolved Hide resolved
docs/src/connectors/sign_convention.md Outdated Show resolved Hide resolved
@ChrisRackauckas ChrisRackauckas merged commit 3a8fb0a into main May 2, 2024
10 of 17 checks passed
@ChrisRackauckas ChrisRackauckas deleted the bgc/sign_convention branch May 2, 2024 16:23
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.

4 participants