raven
raven copied to clipboard
idaholab
WIP: Alfoa/feature selection
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
What are the significant changes in functionality due to this change request?
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 <RunInfo> 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_guideandraven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.
@aalfonsi Will you continue to work on this PR? or do you want me to assign this PR to another developer?
Hi @aalfonsi, we have changed the way to import raven modules, the framework is renamed to ravenframework in order to be able to allow pip to install raven. You can check the PR #1784 for the details, and the following is drafted by Josh for the changes, please let me know if you have any questions.
Suggested email:
In order to make a raven pip package, RAVEN's code needs to be inside a single package. This requires changing all the imports, since now they are inside ravenframework, instead of just at the top level. In addition, the directory framework has been renamed to ravenframework.
This will require changes in three things.
Any imports of things that used to be in framework, now need to have ravenframework explicitly mentioned.
Direct use of Driver.py can no longer be done, since the imports will not be setup correctly. Use raven_framework.py instead.
Pickle files have the package information embedded inside, so they will need to be regenerated.
This is pull request: https://github.com/idaholab/raven/pull/1784
The following are example modifications for framework as a package in the source code.
Relative (use . if in ravenframework/, .. if in ravenframework/someDir/, ... if in ravenframework/someDir/anotherDir/ and so forth.
Example for something that is in a directory inside of ravenframework:
Original:
import Files
New:
from .. import Files
Original:
from utils import mathUtils
New:
from ..utils import mathUtils
Original:
import utils.TreeStructure
New:
from ..utils import TreeStructure
(and modify all uses of utils.TreeStructure to TreeStructure)
Original:
import utils.TreeStructure as TS
New:
from ..utils import TreeStructure as TS
Ravenframework imports:
Original:
import Files
New:
from ravenframework import Files
Original:
from utils import mathUtils
New:
from ravenframework.utils import mathUtils
Original:
import utils.TreeStructure
New:
from ravenframework.utils import TreeStructure
(and modify all uses of utils.TreeStructure to TreeStructure)
Original:
import utils.TreeStructure as TS
New:
from ravenframework.utils import TreeStructure as TS
Importlib (Example from ravenframework/Models/Code.py )
Original:
importlib.import_module("CodeInterfaces")
New:
importlib.import_module("..CodeInterfaces", "ravenframework.Models")
(Note the second parameter package anchor for finding relative packages.)
Driver.py can no longer be used from the python executable.
Original:
python framework/Driver.py "${ARGS[@]}"
New:
python raven_framework.py "${ARGS[@]}"
or
raven_framework "${ARGS[@]}"
@aalfonsi We are excited about this new feature and thanks a lot for your contribution. You mentioned this PR is close to be complete and be ready to be reviewed. Could you resolve the conflict, and remove the "WIP" so we can test the new feature? Once there is no issue about the test, we will start to review it.
@aalfonsi We are excited about this new feature and thanks a lot for your contribution. You mentioned this PR is close to be complete and be ready to be reviewed. Could you resolve the conflict, and remove the "WIP" so we can test the new feature? Once there is no issue about the test, we will start to review it.
@wangcj05 I cleaned up most of the branch. If you want to start the review.
@aalfonsi Thanks for your contribution, I will start to review your this PR next week.
@aalfonsi FYI, there are several tests failed for this PR. See the following file:
@wangcj05 I addressed all the comments made by you and Joshua. In addition I added tests for all the possible combinations of options I added.
If you are good, this can be merged.
@wangcj05 I addressed all the comments made by you and Joshua. In addition I added tests for all the possible combinations of options I added.
If you are good, this can be merged.
Great! Thanks for your fantastic contribution. I will take a look at your modifications.
@wangcj05 I addressed all the comments made by you and Joshua. In addition I added tests for all the possible combinations of options I added. If you are good, this can be merged.
Great! Thanks for your fantastic contribution. I will take a look at your modifications.
These modifications will cause failures in some plugins (all the ones that use pre-serialized surrogate models for their tests (e.g. HERON, FARM, etc.))
@wangcj05 tests passing (I had to re-serialize the ROM in the RAVEN tutorial with protocol=4 since in Windows RAVEN uses python 3.7 that does not support protocol 5 (used by Mac/linux since python installed > 3.7)
Strange, b3e4f43ed37e62205e645cd97b919f46eb3754ec the plugin tests on Centos 8 worked, but on 032452da19d3c1a60b06af61e058a9a6cbb9c930 they didn't.
Strange, b3e4f43 the plugin tests on Centos 8 worked, but on 032452d they didn't.
plugins that rely on serialized roms should all fail... (since the internal rom structure changed).... O_O Strange .
Anyhow do you guys have any other comment on this MR (that I can try to address tonight)?
Strange, b3e4f43 the plugin tests on Centos 8 worked, but on 032452d they didn't.
plugins that rely on serialized roms should all fail... (since the internal rom structure changed).... O_O Strange .
To be more specific, the plugin tests (with the exception of the ExamplePlugin) completely do not run in the newer versions. See for example: https://civet.inl.gov/job/1344686/
Strange, b3e4f43 the plugin tests on Centos 8 worked, but on 032452d they didn't.
plugins that rely on serialized roms should all fail... (since the internal rom structure changed).... O_O Strange .
To be more specific, the plugin tests (with the exception of the ExamplePlugin) completely do not run in the newer versions. See for example: https://civet.inl.gov/job/1344686/
Strange...(it seems a tester issue)... I do not see why this MR would cause that
Strange, b3e4f43 the plugin tests on Centos 8 worked, but on 032452d they didn't.
plugins that rely on serialized roms should all fail... (since the internal rom structure changed).... O_O Strange .
To be more specific, the plugin tests (with the exception of the ExamplePlugin) completely do not run in the newer versions. See for example: https://civet.inl.gov/job/1344686/
Strange...(it seems a tester issue)... I do not see why this MR would cause that
It is because of removing the from future import in xmlUtils. I added a comment. Feel free to cherry-pick/commit --amend this commit: 2143f9fdbf3b086a56c6124c1c61d45564d92d71
Strange, b3e4f43 the plugin tests on Centos 8 worked, but on 032452d they didn't.
plugins that rely on serialized roms should all fail... (since the internal rom structure changed).... O_O Strange .
To be more specific, the plugin tests (with the exception of the ExamplePlugin) completely do not run in the newer versions. See for example: https://civet.inl.gov/job/1344686/
Strange...(it seems a tester issue)... I do not see why this MR would cause that
It is because of removing the from future import in xmlUtils. I added a comment. Feel free to cherry-pick/commit --amend this commit: 2143f9f
shouldn't it be only for Python 2 to Python 3? Is still needed?
Strange, b3e4f43 the plugin tests on Centos 8 worked, but on 032452d they didn't.
plugins that rely on serialized roms should all fail... (since the internal rom structure changed).... O_O Strange .
To be more specific, the plugin tests (with the exception of the ExamplePlugin) completely do not run in the newer versions. See for example: https://civet.inl.gov/job/1344686/
Strange...(it seems a tester issue)... I do not see why this MR would cause that
It is because of removing the from future import in xmlUtils. I added a comment. Feel free to cherry-pick/commit --amend this commit: 2143f9f
shouldn't it be only for Python 2 to Python 3? Is still needed?
Oh O_o I saw your comment... Are you guys planning to "upgrade " the machines?
First part of comments. I have reviewed part of the code, and have provided some comments for you to consider. In addition, is it possible to reduce the test size? For example, there are many csv files have been provided. @aalfonsi
Yes I cannot really reduce the "number of time steps" (Since it is the only way to capture the step function of the test" but I can reduce the number of samples (csvs)... Let me do it now
First part of comments. I have reviewed part of the code, and have provided some comments for you to consider. In addition, is it possible to reduce the test size? For example, there are many csv files have been provided. @aalfonsi @wangcj05 I reduced the CSVs from 72 to 8.
I think I addressed (or answered) to all of your comments. Let me know if you have additional comments