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

[DEFECT] Pickled RAVEN ROM outside of RAVEN non-array inputs #1962

Closed
13 tasks done
dgarrett622 opened this issue Sep 13, 2022 · 6 comments · Fixed by #1963
Closed
13 tasks done

[DEFECT] Pickled RAVEN ROM outside of RAVEN non-array inputs #1962

dgarrett622 opened this issue Sep 13, 2022 · 6 comments · Fixed by #1963

Comments

@dgarrett622
Copy link
Collaborator

dgarrett622 commented Sep 13, 2022

Thank you for the defect report

Defect Description

When using a ROM built in RAVEN outside of RAVEN (in a Python script or Jupyter notebook), passing in a float as input should result in the float being recast as a numpy array. This does not work.

This was noticed with a DMDc ROM from RAVEN when passing in the initial state values. There is a check for floats (SupervisedLearning.py line 339), but it does not work correctly. Also, an error will be thrown if an integer is passed in as an input.

Steps to Reproduce

  1. Create and pickle a ROM
  2. Load the ROM outside of RAVEN
  3. Try passing in a float as one of the inputs
  4. See the failure

Expected Behavior

ROM should act as any ordinary function, pass in inputs, return outputs.

Screenshots and Input Files

No response

OS

Windows

OS Version

No response

Dependency Manager

CONDA

For Change Control Board: Issue Review

  • Is it tagged with a type: defect or task?
  • Is it tagged with a priority: critical, normal or minor?
  • If it will impact requirements or requirements tests, is it tagged with requirements?
  • If it is a defect, can it cause wrong results for users? If so an email needs to be sent to the users.
  • Is a rationale provided? (Such as explaining why the improvement is needed or why current code is wrong.)

For Change Control Board: Issue Closure

  • If the issue is a defect, is the defect fixed?
  • If the issue is a defect, is the defect tested for in the regression test system? (If not explain why not.)
  • If the issue can impact users, has an email to the users group been written (the email should specify if the defect impacts stable or master)?
  • If the issue is a defect, does it impact the latest release branch? If yes, is there any issue tagged with release (create if needed)?
  • If the issue is being closed without a pull request, has an explanation of why it is being closed been provided?
@joshua-cogliati-inl
Copy link
Contributor

joshua-cogliati-inl commented Sep 15, 2022

I get why SupervisedLearning.py line 339 is a problem if an integer is passed in, but what is the problem if a float is passed in?
As in:

>>> type(np.array(3.0)).__name__ == 'float'
False
>>> type(3.0).__name__ == 'float'
True

As in at a quick glance at the code, the bottom one is the one that needs the conversion. But maybe I am missing something?

(And the new code in #1963 is fine with me, I am just trying to understand why it also fixes floats)

@dgarrett622
Copy link
Collaborator Author

I get why SupervisedLearning.py line 339 is a problem if an integer is passed in, but what is the problem if a float is passed in? As in:

>>> type(np.array(3.0)).__name__ == 'float'
False
>>> type(3.0).__name__ == 'float'
True

As in at a quick glance at the code, the bottom one is the one that needs the conversion. But maybe I am missing something?

(And the new code in #1963 is fine with me, I am just trying to understand why it also fixes floats)

Sometimes a float is actually a numpy.float64 or other variant. in that case type(x).__name__ results in float64. The value still needs to be cast to an array, but it failed the check.

A second issue occurs later for the first case, np.array(3.0).shape yields (). Downstream of this code, RAVEN makes an attempt to access the array using an index, which is not possible when the array has shape ().

@joshua-cogliati-inl
Copy link
Contributor

joshua-cogliati-inl commented Sep 15, 2022

Okay, so to approve this, I need answers to: " If it is a defect, can it cause wrong results for users? " and "If the issue is a defect, is the defect tested for in the regression test system? (If not explain why not.)" (A two sentence answer is sufficient.)

@dgarrett622
Copy link
Collaborator Author

dgarrett622 commented Sep 15, 2022

Okay, so to approve this, I need answers to: " If it is a defect, can it cause wrong results for users? " and "If the issue is a defect, is the defect tested for in the regression test system? (If not explain why not.)" (A two sentence answer is sufficient.)

It works fine when run inside of RAVEN and so this should not produce wrong results for users.

I noticed this because I unpickled a RAVEN ROM and attempted to use the ROM as a Python function in an external script. We don't currently have a test for this use case. Is this in the scope of RAVEN tests? If so, the best way to test this would probably be a unit test for ROM inputs.

@joshua-cogliati-inl
Copy link
Contributor

Hm, I am checking if any regression touches this already with:

--- a/ravenframework/SupervisedLearning/SupervisedLearning.py
+++ b/ravenframework/SupervisedLearning/SupervisedLearning.py
@@ -337,6 +337,7 @@ class SupervisedLearning(BaseInterface):
     for index in range(len(values)):
       # If value is a float or int, convert to numpy array for evaluation
       if isinstance(values[index], (float, int)):
+        a = 1/0
         values[index] = np.array([values[index]])
       resp = self.checkArrayConsistency(values[index], self.isDynamic())
       if not resp[0]:

@joshua-cogliati-inl
Copy link
Contributor

Regression test added, so this now can be closed with #1963
I think an email about this defect or the pull request fixing might be useful, but is optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants