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

Refactor PiecewiseBase add coarsened tabulated linear function #14221

Merged
merged 5 commits into from
Dec 24, 2019

Conversation

dschwen
Copy link
Member

@dschwen dschwen commented Oct 23, 2019

I had some refactoring to do on piecewise function. I will need help fixing app fallout (Mastodon in particular). This PR adds a new Piecewise Linear function that performs a coarsening of the supplied data. (this makes sense mainly for data supplied as CSV files)

Refs #14220

@moosebuild
Copy link
Contributor

moosebuild commented Oct 23, 2019

Job Documentation on a65861b wanted to post the following:

View the site here

This comment will be updated on new commits.

@dschwen dschwen force-pushed the coarse_function_14220 branch 2 times, most recently from 9165efd to b449763 Compare October 24, 2019 03:09
@permcody
Copy link
Member

Looks ok - but I bet you can fix the breakage before this goes in.

@dschwen
Copy link
Member Author

dschwen commented Oct 24, 2019

I cannot fix pronghorn (no idea where those exodiffs come from...) or mastodon (ping @sveerara), but I fixed bison.

@dschwen
Copy link
Member Author

dschwen commented Oct 29, 2019

coarsened

Plot from the test file. Reduced from 10000 to 10 points (using a tolerance of 0.1).

@dschwen dschwen force-pushed the coarse_function_14220 branch 3 times, most recently from 5c43315 to e2b8401 Compare October 30, 2019 12:46
@dschwen
Copy link
Member Author

dschwen commented Oct 30, 2019

@milljm the Minimum gcc civet environment is missing the numpy python package (which is present everywhere else)

functions/coarsened_piecewise_linear.prepare_data: Working Directory: /opt/civet/build_0/moose/test/tests/functions/coarsened_piecewise_linear
functions/coarsened_piecewise_linear.prepare_data: Running command: ./generate_data.py
functions/coarsened_piecewise_linear.prepare_data: Traceback (most recent call last):
functions/coarsened_piecewise_linear.prepare_data:   File "./generate_data.py", line 3, in <module>
functions/coarsened_piecewise_linear.prepare_data:     import numpy as np
functions/coarsened_piecewise_linear.prepare_data: ImportError: No module named numpy
functions/coarsened_piecewise_linear.prepare_data: 
functions/coarsened_piecewise_linear.prepare_data: ################################################################################
functions/coarsened_piecewise_linear.prepare_data: Tester failed, reason: CODE 1
functions/coarsened_piecewise_linear.prepare_data: 
    FAIL functions/coarsened_piecewise_linear.prepare_data FAILED (CODE 1)

[./prepare_data]
type = RunCommand
command = './generate_data.py'
requirement = "Generate the fine tabulated function data for the coarsened_piecewise_linear test"
Copy link
Member

Choose a reason for hiding this comment

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

Add the following, so this test is properly skipped, if numpy is not available:

required_python_packages = 'numpy'

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to just modify the script to not use numpy if that's an issue? Do we have systems where numpy isn't available?

Copy link
Member Author

@dschwen dschwen Dec 13, 2019

Choose a reason for hiding this comment

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

I don't think there are systems where MOOSE is available, but it is impossible to get numpy. The test boxes certainly have it and my priority is to make those run as efficiently as possible.

@dschwen dschwen changed the title Refactor PiecewiseBase Refactor PiecewiseBase add coarsened tabulated linear function Oct 30, 2019
@dschwen
Copy link
Member Author

dschwen commented Oct 30, 2019

@sveerara, Mastodon is the only app that fails now with

/opt/civet/build_0/mastodon/src/transfers/HazardCurveTransfer.C:4:10: fatal error: Piecewise.h: No such file or directory
 #include "Piecewise.h"
          ^~~~~~~~~~~~~

I think the fix is simply to include PiecewiseBase.h now.

@sveerara
Copy link
Contributor

@dschwen, I will add this fix now to MASTODON. Thanks!

@sveerara
Copy link
Contributor

I have added a merged request to Mastodon (258) to be merged after this one gets merged.

@dschwen
Copy link
Member Author

dschwen commented Nov 1, 2019

@bwspenc would you mind taking a look at this to wrap it up?

@permcody
Copy link
Member

permcody commented Nov 4, 2019

@bwspenc - ping

bwspenc
bwspenc previously requested changes Nov 6, 2019
Copy link
Contributor

@bwspenc bwspenc left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Just minor stuff. It looks like some conflicts need to be resolved too. Sorry about not getting to this sooner.

framework/include/functions/CoarsenedPiecewiseLinear.h Outdated Show resolved Hide resolved
framework/include/utils/PointReduction.h Outdated Show resolved Hide resolved
framework/include/functions/PiecewiseLinearBase.h Outdated Show resolved Hide resolved
framework/src/utils/PointReduction.C Outdated Show resolved Hide resolved
framework/src/utils/PointReduction.C Outdated Show resolved Hide resolved
test/tests/functions/coarsened_piecewise_linear/tests Outdated Show resolved Hide resolved
[./prepare_data]
type = RunCommand
command = './generate_data.py'
requirement = "Generate the fine tabulated function data for the coarsened_piecewise_linear test"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to just modify the script to not use numpy if that's an issue? Do we have systems where numpy isn't available?

@dschwen
Copy link
Member Author

dschwen commented Dec 6, 2019

Mastodon fix is still needed, but Swetha mentioned that is has been ready for a month.

@dschwen dschwen requested a review from bwspenc December 6, 2019 21:56
@permcody
Copy link
Member

permcody commented Dec 9, 2019

@dschwen - I can merge the Mastodon patch (1 liner). It has to go in after this one though.

@dschwen
Copy link
Member Author

dschwen commented Dec 9, 2019

Thanks @permcody , the failure above is a seemingly unrelated python unittest TIMEOUT

@dschwen
Copy link
Member Author

dschwen commented Dec 10, 2019

@bwspenc could you please re-review this?

@dschwen dschwen dismissed bwspenc’s stale review December 13, 2019 15:49

Please re-review. I think we can reasonably request numpy to be installed in a MOOSE environment.

@dschwen
Copy link
Member Author

dschwen commented Dec 24, 2019

♫ 🎶♪ All I want for Christmas is @bwspenc's rev-youuuuuuuu... ♫ 🎶♪

@dschwen
Copy link
Member Author

dschwen commented Dec 24, 2019

carey_xmas3

@bwspenc bwspenc merged commit 9a91b04 into idaholab:next Dec 24, 2019
@bwspenc
Copy link
Contributor

bwspenc commented Dec 24, 2019

Merry Christmas!

"All function data, supplied in abscissa, ordinate pairs");
params.addParam<std::vector<Real>>("x", "The abscissa values");
params.addParam<std::vector<Real>>("y", "The ordinate values");
params.addParam<FileName>("data_file", "File holding CSV data for use with Piecewise");
Copy link
Contributor

Choose a reason for hiding this comment

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

Aie! How did these sneak in...I created PiecewiseBase so I could make a custom linear function with inputs not like x and y, but rather ramp_rates, and hold_times, while still allowing me to pass it to IterationAdaptiveDT, which requires a PiecewiseBase. I think I need to make a PiecewiseBaseBase now :(

It looks like PiecewiseBase is only used in PiecewiseConstant and PiecewiseLinearBase, at least in MOOSE. Maybe I can just move these input parameters into those two files...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you took away Piecewise which gave me the flexibility I needed to not rely on input parameters to build my interpolation @dschwen!

Copy link
Member Author

@dschwen dschwen Jan 31, 2022

Choose a reason for hiding this comment

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

@tophmatthews, is it maybe this PR? Was the problem you raised ever solved?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I thought I fixed this...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, that isn't me, @dschwen ...sorry @topher

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what he deserves for snatching that username away from you :-D

Copy link
Contributor

Choose a reason for hiding this comment

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

Right! Doesn't seem to be active either. :)

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

Successfully merging this pull request may close these issues.

None yet

7 participants