-
Notifications
You must be signed in to change notification settings - Fork 64
Issue1436 add pvt model #1446
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
Issue1436 add pvt model #1446
Conversation
|
@LoneMeertens thanks for creating this new model, it's a nice development! :-) Regarding the large files: if the model is developed and validated within IDEAS, I think it is best to add all original data. Moreover, the files are static. Just make sure that you only add the data that you use for validation. I'll have a first look at your implementation now, I am curious ;-) |
lucasverleyen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LoneMeertens I had a quick view at your models, looking great :-) I have added some (first) suggestions.
| @@ -0,0 +1,2 @@ | |||
| PVT1 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of 1 and 2? I would keep the naming descriptive, e.g. WithRearInsulation and WithoutRearInsulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used 'PVT1' and 'PVT2' in the paper referenced, as it seemed the most straightforward and user-friendly way to help readers quickly connect the two datasets. However, since most users likely won’t make the effort to look up the paper, it seems better to use 'WithRearInsulation' and 'WithoutRearInsulation' instead, thanks for the suggestion!
IDEAS/Fluid/PvtCollectors/BaseClasses/EN12975HeatLoss_QuasiDynamic.mo
Outdated
Show resolved
Hide resolved
IDEAS/Fluid/PvtCollectors/BaseClasses/PartialEN12975HeatLoss_QuasiDynamic.mo
Outdated
Show resolved
Hide resolved
IDEAS/Fluid/PvtCollectors/BaseClasses/PartialEN12975HeatLoss_QuasiDynamic.mo
Outdated
Show resolved
Hide resolved
jelgerjansen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LoneMeertens I did a review of all models except those in the Validation package. I propose you first have a look at my comments, and then use that feedback for the Validation package as well.
IDEAS/Fluid/PvtCollectors/BaseClasses/EN12975HeatLoss_QuasiDynamic.mo
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,67 @@ | |||
| within IDEAS.Fluid.PvtCollectors.BaseClasses; | |||
| model EN12975HeatLoss_QuasiDynamic | |||
| "Model to calculate the quasi-dynamic heat loss of a pvt/solar collector following the ISO 9806:2013 quasi-dynamic method" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The model could also be used in the solar thermal collector model. Maybe we should have a thought on how to use this BaseClass model for both PVT and STC in the future (could be discussion in the Modelica Working Group)
- The model is called EN12975, but the description only mentions ISO 9806:2013. What is the link with EN12975?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! The model originally extended from EN12975HeatLoss (SolarCollectors package), which implements the steady-state method from the now-outdated EN 12975 standard. However, the logic in this model follows the ISO 9806:2013 quasi-dynamic method, which has replaced EN 12975-2 as the current international standard. To reflect this, I’ve renamed the model to ISO9806QuasiDynamicHeatLoss.
That said, this does raise a broader question: since ISO 9806 has fully replaced EN 12975, should we also consider renaming the STC model to avoid confusion and align all naming with the current standard?
IDEAS/Fluid/PvtCollectors/BaseClasses/EN12975HeatLoss_QuasiDynamic.mo
Outdated
Show resolved
Hide resolved
IDEAS/Fluid/PvtCollectors/BaseClasses/EN12975HeatLoss_QuasiDynamic.mo
Outdated
Show resolved
Hide resolved
…2975QuasiDynamicHeatLoss.mo
… area" in EN12975QuasiDynamicHeatLoss.mo to match datasheet convention
…low naming convention
…s to follow naming convention" This reverts commit 324732a.
…N12975QuasiDynamicHeatLoss.mo
…GloHorNom in PartialPvtCollector
…tauAlphaEff parameter in PartialPvtCollector
…fier in PartialPvtCollector
…PartialPvtCollector
…nd from PartialSolarCollector and remove dependency on PartialPVTCollector
…ynamicHeatLoss and add additional terms
…zational conventions
|
@jelgerjansen Thank you for the elaborate review, it was very useful to help create a more compact and clear structure within the PVTCollector package. I’ve now implemented all the suggested changes across the models, including those in the Validation package. Here’s a summary of the main structural changes:
Follow-up points:
|
… parameter to ElectricalPVT block
…y with mos scripts
…os script compatibility
6f91722 to
65a2772
Compare
aeb3965 to
8989e21
Compare
|
@jelgerjansen @lucasverleyen Do you still have time to have a look at this PR before the IDEAS release? Follow-up points (taken from above and extended):
Let me know your thoughts. |
|
|
Fixes #1436 .
This PR implements a validated dynamic PVT collector model based on EN12975 and includes electrical coupling, as described in issue #1436
Two large files were added to support validation:
These contain 5-second interval data. They were uploaded using Git LFS to bypass GitHub's file size limit. However, I'm concerned about the long-term relevance and maintainability of including such large files in the main repository. Suggestions on how to best handle this (e.g., external hosting, downsampling, separate data repo) are welcome.
@jelgerjansen: If you have comments about structure or integration with the broader library, please let me know.