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

LSTM Regression #1519

Merged
merged 26 commits into from
May 10, 2021
Merged

LSTM Regression #1519

merged 26 commits into from
May 10, 2021

Conversation

joshua-cogliati-inl
Copy link
Contributor

@joshua-cogliati-inl joshua-cogliati-inl commented Apr 19, 2021


Pull Request Description

What issue does this change request address?

Closes #1540

What are the significant changes in functionality due to this change request?

Adds a LSTM regression (we have an existing LSTM classifier)


For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass, including run_tests, pylint, manual building and xsd tests. If there are changes to Simulation.py or JobHandler.py the qsub tests must pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test. If new development on the internal JobHandler parallel system is performed, a cluster test must be added setting, in XML block, the node <internalParallel> to True.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added is the the analytic documentation updated/added?
  • 9. If any test used as a basis for documentation examples (currently found in raven/tests/framework/user_guide and raven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.

@joshua-cogliati-inl joshua-cogliati-inl changed the title Cogljj/lstm regression LSTM Regression Apr 19, 2021
@moosebuild
Copy link

Job Test mac on c2647a1 : invalidated by @joshua-cogliati-inl

restarted civet

@moosebuild
Copy link

Job Mingw Test on c2647a1 : invalidated by @joshua-cogliati-inl

restarted civet

from .SupervisedLearning import supervisedLearning
#Internal Modules End--------------------------------------------------------------------------------

class KerasRegression(supervisedLearning):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this class need to be merged with the KerasClassifier class. Most of the functionalities are the same for both Regression and Classifier. I think a new base class can be extracted from these two classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree (this is one of the reasons this is marked Do Not Merge). My plan is to make a KerasBaseClass, and put the common things in KerasRegression and KerasClassifier in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a base class and pulled some common features out to it. I am still deciding if I need to work a bit more to get more common functions out or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have extracted pretty much all of the common functionality into a base class. So if you want to take a look now, it would be welcome.

@moosebuild
Copy link

Job Mingw Test on e4a31fa : invalidated by @joshua-cogliati-inl

restarted civet

@moosebuild
Copy link

Job Test Fedora 31 on dfad1fa : invalidated by @joshua-cogliati-inl

failed tests/framework/ensembleModelTests/testEnsembleModelLateralInsertions

@joshua-cogliati-inl
Copy link
Contributor Author

@wangcj05 or @alfoa This is ready for more review. Thanks.

Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

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

Some minor comments for your consideration. It is a good start to create the base class for neural network.

@wangcj05
Copy link
Collaborator

wangcj05 commented May 4, 2021

When I compared the KerasLSTMClassifier and KerasLSTMRegression, they share most of the functions. This suggests to me it is probably better to combine these two classes. I suggest we have group discussion. @alfoa @joshua-cogliati-inl

self.initOptionDict[layerName]['return_sequences'] = True
self.raiseAWarning('return_sequences is resetted to True for layer',layerName)

def _preprocessInputs(self,featureVals):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wangcj05 Hm, I think I could move _preprocessInputs to KerasBase, if you agree that would be a good idea. I think that removes the major redundancy in KerasLSTMRegression and KerasLSTMCLassifier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is tied to LSTM. It is not general all neural network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, then I could just make a LSTM utils, and put the function in that, and use it in both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tho' since @alfoa is going to be making changes to this soon, maybe I should just put a comment in both files that it is identical, and after the changes we can decide if it is worth changing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To me, a comment is fine. I think my comments are more for future. How do we want the DNN classes inheritance look like? @alfoa any more comments? BTW, the issue that I raised can be further discussed and addressed in future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I added a comment to the source code.

@wangcj05
Copy link
Collaborator

@joshua-cogliati-inl qsubs failed, could you take a look?

@moosebuild
Copy link

Job Test qsubs on 9b7b418 : invalidated by @joshua-cogliati-inl

Failed tests/framework/Samplers/Categorical/RestartMissingVars

@moosebuild
Copy link

Job Test qsubs on 9b7b418 : invalidated by @joshua-cogliati-inl

Failed tests/framework/CodeInterfaceTests/RAVEN/Basic

@wangcj05
Copy link
Collaborator

This PR is good. Checklist is satisfied, and tests are green (OpenSUSE Leap 15 is down for now.).

@wangcj05 wangcj05 merged commit cf0c4f9 into devel May 10, 2021
@wangcj05 wangcj05 deleted the cogljj/lstm_regression branch May 10, 2021 19:58
@wangcj05 wangcj05 added the RAVENv2.1 All tasks and defects that will go in RAVEN v2.1 label May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RAVENv2.1 All tasks and defects that will go in RAVEN v2.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TASK] LSTM Regression
3 participants