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

Fix issues with MeasureManager #720

Merged
merged 41 commits into from
Aug 2, 2024
Merged

Fix issues with MeasureManager #720

merged 41 commits into from
Aug 2, 2024

Conversation

macumber
Copy link
Collaborator

@macumber macumber commented Jul 5, 2024

…ts for first measure are being computed (this can take a long time in failure case). Catch all std::exceptions instead of just RubyExceptions which prevents crash. Fixes #656
@macumber macumber added this to the OpenStudio Application 1.8.0 milestone Jul 5, 2024
@macumber
Copy link
Collaborator Author

macumber commented Jul 12, 2024

@jmarrec when updating a model with the old results measure, we probably need to swap the new one out, currently there is a crash due to this. The exception is caught in this branch now at least. However, there are going to be many measures that might need to be updated. So maybe it's best to not update the results measure, we can just show the errors and make users delete/replace measures themselves. This is going to be something we need to address in the release notes.

# Conflicts:
#	ConanInstall.cmake
#	src/openstudio_lib/OSDocument.cpp
@macumber macumber requested a review from jmarrec July 14, 2024 19:21
@macumber
Copy link
Collaborator Author

@jmarrec if you have a chance to review this that would be good, the PR is not too big if you ignore src/openstudio_app/Resources/ which is just copies of measures from BCL

@jmarrec
Copy link
Collaborator

jmarrec commented Jul 15, 2024

I'd probably have liked if the "Example > Shoebox Model" was isolated into its own PR.

@macumber
Copy link
Collaborator Author

macumber commented Jul 15, 2024

I'd probably have liked if the "Example > Shoebox Model" was isolated into its own PR.

Agreed, sorry I was in a rush, I can move it to a new PR if you'd like

FYI, I did merge openstudiocoalition/openstudio-coalition-measures#3 and release https://github.com/openstudiocoalition/openstudio-coalition-measures/releases/tag/v1.8.0 to support this

@@ -275,7 +276,6 @@ class OpenStudioApp : public OSAppBase
QTranslator m_qtTranslator;
QTranslator m_qtBaseTranslator;
QString m_currLang;
bool m_useClassicCLI;
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Remove unused m_useClassicCLI, cannot use classic CLI for MeasureManager any more" (65af11b)

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This issue prevents using the classic CLI for the measure manager now: NREL/OpenStudio#5212

I figure it's probably not really worth supporting using classic for the measure manager in the future either, what do you think? You can still use the classic option for the run though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really like seeing these files here. Should we rely on the BCL to download these as needed?

Copy link
Collaborator Author

@macumber macumber Jul 16, 2024

Choose a reason for hiding this comment

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

I wanted to make these measures/example models part of the installer, so it is just ready to go with no download needed. We could fetch the measures/example models from https://github.com/openstudiocoalition/openstudio-coalition-measures at build time and keep them in the build directory rather than checking them in to this repo. They'd still be duplicates in the installer. Does that sound better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed these measures from this repo, they are now pulled from https://github.com/openstudiocoalition/openstudio-coalition-measures at configure time

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now we have two copies of the openstudio_results measure in source: in the Resources/ and Resources/ShoeboxExample/measures folders

Copy link
Collaborator Author

@macumber macumber Jul 16, 2024

Choose a reason for hiding this comment

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

I guess we could add the report measure to the ShoeboxExample model at run time. I had wanted to keep the model exactly the same as in https://github.com/openstudiocoalition/openstudio-coalition-measures/tree/main/models/ShoeboxExample, that way it would be simple to extend to other example models in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed these measures from this repo, they are now pulled from https://github.com/openstudiocoalition/openstudio-coalition-measures at configure time

…to measures_dir. Use both for now in case that changes.
# Conflicts:
#	translations/OpenStudioApp_ar.ts
#	translations/OpenStudioApp_ca.ts
#	translations/OpenStudioApp_de.ts
#	translations/OpenStudioApp_el.ts
#	translations/OpenStudioApp_es.ts
#	translations/OpenStudioApp_fa.ts
#	translations/OpenStudioApp_fr.ts
#	translations/OpenStudioApp_he.ts
#	translations/OpenStudioApp_hi.ts
#	translations/OpenStudioApp_it.ts
#	translations/OpenStudioApp_ja.ts
#	translations/OpenStudioApp_pl.ts
#	translations/OpenStudioApp_vi.ts
#	translations/OpenStudioApp_zh_CN.ts
# Conflicts:
#	src/openstudio_lib/OSDocument.cpp
@macumber
Copy link
Collaborator Author

Ping @jmarrec, do you want to give this a review? If not, I think we could merge and push out an RC1. What do you think?

@jmarrec
Copy link
Collaborator

jmarrec commented Jul 31, 2024

Ping @jmarrec, do you want to give this a review? If not, I think we could merge and push out an RC1. What do you think?

An RC1 for 1.8.0? I'd like to see if we can get an updated CLI with NREL/OpenStudio#5230 fixed first

@macumber
Copy link
Collaborator Author

macumber commented Aug 1, 2024

I don't think we need an updated 1.8.0 package. After I tested, the main issue was with the OpenStudio Results measure which gets downloaded and added to each model by default at startup. I fixed that in the 1.7.2 release. The other BCL menus have some filtering built into them, I think I have handled all the needed cases in this PR such that we don't have to have an update to OS SDK.

@jmarrec jmarrec merged commit 55f5cc3 into develop Aug 2, 2024
8 checks passed
@jmarrec jmarrec deleted the measure_fixes branch August 2, 2024 13:36
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.