-
Notifications
You must be signed in to change notification settings - Fork 133
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
Adding constrained sensing capabilities to the SPSL post processor. Added a test_SPSL_constrained xml for testing. #2346
base: devel
Are you sure you want to change the base?
Conversation
I believe this uses features in sparsesensing that are not yet in a released version, so besides any other work that needs to be done, this should not be merged until the sparsesensing release comes out. |
…9/raven into SPSL_add_constraints Need for testing
…whitespace changes to Sparsesensing.py
Job Test mac on d2dc13b : invalidated by @joshua-cogliati-inl failed in fetch |
Opps, actually failed in set python environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some comments for your consideration.
<Output class="DataObjects" type="DataSet">outPP</Output> | ||
</PostProcess> | ||
<IOStep name="print"> | ||
<Input class="DataObjects" type="DataSet">outPP</Input> | ||
<Input class="DataObjects" type="DataSet">outPP</Input>cd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is cd
a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Thank you for catching that. I have removed cd.
goal.addSub(xValue) | ||
yValue = InputData.parameterInputFactory("yValue", contentType=InputTypes.StringListType, | ||
printPriority=108, | ||
descr=r"""Variable plotted on the Y-axis of the data model""") ##Do we want another Z-value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, do we want a zValue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! In case we have 3D data! Thank you for catching that!
printPriority=108, | ||
descr=r"""X coordinate of the focus of the parabola""") | ||
goal.addSub(a) | ||
xy_coords = InputData.parameterInputFactory("xy_coords", contentType=InputTypes.IntegerListType, ##What is the type for an array? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, some of the comments like "What is the type for an array?" look like they may be out of date.
self.sampleTag = 'RAVEN_sample_ID' # The sample tag | ||
self.center_x = None # The x co-ordinate of center of circle,ellipse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it looks like this only supports one Constrained Region? I think PySensors supports multiple constrained regions?
## TODO: Add GQR for constrained optimization | ||
elif self.optimizer.lower() == 'gqr': | ||
optimizer = ps.optimizers.GQR() | ||
## TODO: Add CCQR for cost constrained optimization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do any of these TODO's need to be fixed?
@@ -0,0 +1,136 @@ | |||
<?xml version="1.0" ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file does not seem to be used in the tests yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like new tests have been added, so maybe this file can be removed?
<constraintOption>"exact_n"</constraintOption> | ||
<ConstrainedRegions type = "circle"> | ||
<center_x></center_x> #center coordinates instead (x,y) (r, \theta) | ||
<center_y></center_y> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments in xml are done using <!--your comment-->
center_x and center_y seems not to be recognized
image = 'classificationOptiTwist/mySensorPlot_scatter-scatter.png classificationOptiTwist/mySensorPlot3D_scatter.png' | ||
required_libraries = 'imageio' | ||
rel_err = 0.01 | ||
[../] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only review the image with a relative error of 0.01 which is too small for image comparison. We need to test the csv and the image. rel_err
for image can be 0.1
…nd polygon constraints
Job Test qsubs sawtooth on cfba5ed : invalidated by @niharika2999 |
Job Test qsubs sawtooth on cfba5ed : invalidated by @wangcj05 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.DS_Store and .ravenStatus should be removed from the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a gold file? (Also for the other outPP.csv files)
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Closes #2347
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.
<internalParallel>
to True.raven/tests/framework/user_guide
andraven/docs/workshop
) have been changed, the associated documentation must be reviewed and assured the text matches the example.