Conversation
|
There is a 64MB dump of test-data that is a duplicate of the test-data-set used for ecl2df. For presentvalue.py, probably only the UNSMRY file is strictly needed. |
|
Cool! I know it's just a WIP, but would you mind removing the shebang? |
|
Open to suggestions on how to solve the relative path issue for test data. Is it feasible to capture stdout when testing? |
subscript/tests/test_presentvalue.py
Outdated
| "presentvalue", | ||
| "--discountto", | ||
| "2001", | ||
| "tests/data/reek/eclipse/model/2_R001_REEK-0.DATA", |
There was a problem hiding this comment.
How about os.path.join(__file__, "data", .....). We can even add a utils inside the test-module that fetches test data tests.utils.get_test_data("reek", "eclipse", "model", "2_R001_REEK-0.DATA").
There was a problem hiding this comment.
Is it feasible to capture stdout when testing?
Yes, but how about writing a test of the main function first?
|
I think I want to leave this as is, for my first conversion. But docs should probably be added, here the simple solution is to add a short descriptive string to the wiki, where it is already documented. https://wiki.equinor.com/wiki/index.php/ResScript:Python:Scripts:presentvalue Or/and employ sphinx-argparse? |
That sounds like a good idea! |
|
Are we sure this functionality is not covered by the NPV job in |
| import pandas | ||
| import sys | ||
|
|
||
| # import resscript.header as header |
There was a problem hiding this comment.
We probably need to make a replacement for this + logging.
| @@ -0,0 +1,508 @@ | |||
| """ | |||
There was a problem hiding this comment.
Should probably remove this.
There was a problem hiding this comment.
(The documentation at the top with out names etc, as well as the link to the script documentation: that should be some new Sphinx site)
| parser.add_argument("--quiet", "-q", action="store_true", help="Be quiet") | ||
| args = parser.parse_args() | ||
|
|
||
| # if not args.quiet: |
There was a problem hiding this comment.
Hold until we have a new header solution.
This script has break-even-calculations also, which I could not find by scrolling the spinningjenny source. But it is already ported, so not much more we need to do. |
You are right, that is not in the current implementation as it was not necessary from an Everest perspective (in the forward models at least). |
|
I think we should merge this for now, and continue to other scripts. Review requested :) |
|
My recommendation is that the minimum effort put into this PR before it being merged is that you rewrite the main function so that it either return values as requested or throws an exception. In addition you can use the However, I'm not the maintainer of this code, so do as you like with my recommendations... |
|
Thanks, those are good suggestions for improval, but I prefer to put them in a separate issue for later triaging. I added a numerical test that will ensure that it at least computes correctly on a sample test. Test code clearly illustrates some refactoring need. |
Then you can merge it if you like. I would encourage you to make an issue regarding the improvements ;) |
|
"Merging can be performed automatically with 1 approving review." ;) |
|
I would prefer the commits being remade; first commit for the testdata, second commit for the code, third commit for fixing the code according to black, flake, etc, fourth commit for the testing or something like that. We don't need the commits that are It should be
|
|
Why are there raw |
|
(I don't have the knowledge to reorder/selectively squash the commits in an effective manner) |
berland
force-pushed
the
presentvalue
branch
from
dc62bc9 to
f999c7e
Compare
(fixed) |
* 2to3 * Blacken * Encapsulate code in def main() * Precise exceptions
berland
force-pushed
the
presentvalue
branch
from
f999c7e to
54b7ef0
Compare
berland
force-pushed
the
presentvalue
branch
from
54b7ef0 to
9b99bc8
Compare
4 participants
Minimal amount of work to get it py3-compatible;