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 VAVTurndownDuringReheat Untested outcome #59

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

yunjoonjung-PNNL
Copy link
Collaborator

No description provided.

plt.tight_layout()
plt.savefig(f"{self.results_folder}/Day_plot_aio.png")
print()
if plotday_filtered.size != 0:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@leijerry888 The reason why I added this condition is to prevent an error when the size of the plotday_filtered is 0,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on the discussion, we decided not to make updates for plotting in this PR (Interactive plot PR will handle the issue).


data = [
[True, 350, 0],
[True, 370, 0],
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like this is the only difference from the previous test. Should the error log being asserted in the test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point.

One thing I don't understand is that when I added the following code at the end of the unit test (line 284), the logobs is empty, which I don't understand. I'll keep this PR for now to take more time to find the reason. This shouldn't impact the testing.

with self.assertLogs() as logobs:
    self.assertEqual(
        "ERROR:root:Any `V_dot_VAV_max` value shouldn't be zero.",
        logobs.output[0],
    )

@leijerry888
Copy link
Collaborator

@yunjoonjung-PNNL, don't forget to deal with the "Untested" plotting, just replace it with nan.

@yunjoonjung-PNNL
Copy link
Collaborator Author

@yunjoonjung-PNNL, don't forget to deal with the "Untested" plotting, just replace it with nan.

I'm a bit confused. What's been done in PR #61 was to replace np.nan with Untested. Did you mean I should replace Untested with np.nan?

@leijerry888
Copy link
Collaborator

@yunjoonjung-PNNL, don't forget to deal with the "Untested" plotting, just replace it with nan.

I'm a bit confused. What's been done in PR #61 was to replace np.nan with Untested. Did you mean I should replace Untested with np.nan?

Yes, so that plotting can work without reporting error.

@yunjoonjung-PNNL
Copy link
Collaborator Author

@yunjoonjung-PNNL, don't forget to deal with the "Untested" plotting, just replace it with nan.

I'm a bit confused. What's been done in PR #61 was to replace np.nan with Untested. Did you mean I should replace Untested with np.nan?

Yes, so that plotting can work without reporting error.

Should all the Untested results in all the verification items be replaced as well?

@leijerry888
Copy link
Collaborator

@yunjoonjung-PNNL, don't forget to deal with the "Untested" plotting, just replace it with nan.

I'm a bit confused. What's been done in PR #61 was to replace np.nan with Untested. Did you mean I should replace Untested with np.nan?

Yes, so that plotting can work without reporting error.

Should all the Untested results in all the verification items be replaced as well?

No need to do that. Let's handle it in the super class plotting functions. If no such unit tests are implemented, could you please implement some plotting unit tests with "Untested" in the results to make sure all plotting functions work? Happy to discuss more :)

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.

2 participants