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

Update ModelicaTest/Media.mo: Reduce dp_nominal in TestsWithFluid.Components.PartialTestModel by a factor of 1000 #3918

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

hubertus65
Copy link
Member

reduced dp_nominal in TestsWithFluid.Components.PartialTestModel by a factor of 1000, which makes it reasonable, and will change all reference results in TestsWithFluid.

reduced dp_nominal in TestsWithFluid.Components.PartialTestModel by a factor of 1000, which makes it reasonable, and will change all reference results in TestsWithFluid.
Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

LGTM. A nominal pressure drop of 1 GPa is definitely outlandish. 10 bars is also a lot, but reasonable in some applications.

@beutlich
Copy link
Member

@hubertus65 Is this for #3852? If yes you should self-assign there and link the PR.

@beutlich beutlich changed the title Update Media.mo Update ModelicaTest/Media.mo: Reduce dp_nominal in TestsWithFluid.Components.PartialTestModel by a factor of 1000 Dec 14, 2021
@beutlich beutlich added the L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined label Dec 14, 2021
@AtiyahElsheikh
Copy link
Member

Looks ok. I guess the failed check prevents the merge.

@sjoelund
Copy link
Member

sjoelund commented Feb 2, 2022

The CI seems to work fine for branches that are not on the same repository. I am not sure why, but Jenkins thinks it's not a proper pull request...

@sjoelund
Copy link
Member

sjoelund commented Feb 2, 2022

Ah, no.It shouldn't block the merge. It's the branch job that fails to build. The pr-merge job works fine. So if you're OK with it @AtiyahElsheikh approve the changes.

@AtiyahElsheikh
Copy link
Member

@sjoelund I am ok. But there is no approve button.

@sjoelund
Copy link
Member

sjoelund commented Feb 2, 2022

There should be something saying review and if you expand it, you can approve.

@dietmarw dietmarw removed their request for review February 2, 2022 10:08
@dietmarw
Copy link
Member

dietmarw commented Feb 2, 2022

I've removed myself from review since it is not really my field and I can not judge if the reduction is "reasonable".

@beutlich beutlich linked an issue Feb 2, 2022 that may be closed by this pull request
@dietmarw dietmarw added this to the MSL4.1.0 milestone Jun 23, 2022
@dietmarw dietmarw self-assigned this Jun 23, 2022
@dietmarw dietmarw merged commit 8e2a454 into master Jun 23, 2022
@dietmarw dietmarw deleted the hubertus65-patch-for-TestsWithFluid branch June 23, 2022 08:42
@beutlich beutlich removed the request for review from rfranke June 23, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid models ModelicaTest.Media.TestsWithFluids.MediaTestModels.*
7 participants