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

Issue1880 mover efficiency fix and validation rework #1881

Merged
merged 25 commits into from
Jun 19, 2024

Conversation

hcasperfu
Copy link
Contributor

This closes #1880.

@hcasperfu hcasperfu self-assigned this May 14, 2024
Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

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

See inline comment for missing revision note. Otherwise it looks good (but the CI tests fail which needs to be corrected).

@hcasperfu hcasperfu marked this pull request as ready for review May 15, 2024 23:19
@hcasperfu hcasperfu requested a review from mwetter May 15, 2024 23:20
@hcasperfu
Copy link
Contributor Author

@mwetter - This PR is ready for review.
I obsoleted Movers.Validation.{PowerExact,PowerEuler,PowerSimplified} and made Movers.Validation.{ComparePowerHydraulic,ComparePowerInput,ComparePowerTotal}.

  • ComparePowerHydraulic validates that $\eta = \eta_{hyd} \cdot \eta_{mot}$ under per.powerOrEfficiencyIsHydraulic=true.
  • ComparePowerTotal validates the above under per.powerOrEfficiencyIsHydraulic=true.
  • ComparePowerInput shows that the mover models with different inputs produce the same power estimation results.

I will work on a concurrent PR with Buildings.
Once you approve I will request an external review.

@hcasperfu
Copy link
Contributor Author

In a70a9f5:

@hcasperfu hcasperfu requested a review from Mathadon June 10, 2024 17:50
Copy link
Member

@Mathadon Mathadon left a comment

Choose a reason for hiding this comment

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

@hcasperfu this looks okay, thanks!

@mwetter mwetter self-assigned this Jun 17, 2024
Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

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

This is ready to merge when the CI tests pass.

@mwetter mwetter enabled auto-merge June 19, 2024 01:49
@mwetter mwetter merged commit addf290 into master Jun 19, 2024
3 checks passed
@mwetter mwetter deleted the issue1880_moverEfficiencyFixAndValidationRework branch June 19, 2024 17:36
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.

Correct mover efficiency in unusual configuration and rework validation models
3 participants