search

NREL / GEOPHIRES-X

MIT License
28 stars 24 forks source link

updated well drill cost correlations #165

Closed bpulluta closed 5 months ago

bpulluta commented 6 months ago

Updated well drilling cost correlations to match GETEM correlations based on Sandia Report: https://www.nrel.gov/docs/fy23osti/82771.pdf

softwareengineerprogrammer commented 6 months ago

@bpulluta thanks for submitting!

First step is to get Actions (automated tests) passing, see test failure https://github.com/NREL/GEOPHIRES-X/actions/runs/8380691581/job/22950491702?pr=165#step:5:1221

bpulluta commented 6 months ago

@bpulluta thanks for submitting!

First step is to get Actions (automated tests) passing, see test failure https://github.com/NREL/GEOPHIRES-X/actions/runs/8380691581/job/22950491702?pr=165#step:5:1221

I'll work on getting automated tests working.

@kfbeckers, SAM/GETEM uses large vertical intermediate 1 as the default. Would we want to match it here to also use it as the default?

kfbeckers commented 6 months ago

Yes, once all 16 correlations are incorporated, I would say yes that is a good idea to switch the default to Intermediate 1 - large diameter - vertical

softwareengineerprogrammer commented 6 months ago

Hi @bpulluta, just checking in - I'm assuming you're working on addressing the test failures and filing another revision of this PR to fix, but let me know if you would like any further guidance or info in the meantime.

softwareengineerprogrammer commented 6 months ago

Note: this PR, once approved/merged will probably be sufficient to resolve https://github.com/NREL/GEOPHIRES-X/issues/129 (pending evaluation of whether there are any aspects of that issue that are not addressed in this PR)

bpulluta commented 5 months ago

Awaiting updated example outputs and passing Actions per #165 (review)

@softwareengineerprogrammer @kfbeckers , I updated the cost correlations. Previously, I thought the previous geophires implementation was using ideal correlations but actually it was using baseline correlations. That's why we saw the cost decrease when we updated it to the new correlations. As such, I fixed the implementation to call baseline correlations for the first 4 correlations, this made it consistent and backwards compatible with the previous implementation. All tests are now passing correctly.

Additionally, i added a function to test_csv to help track where in the csv there were differences. Nothing major but it helped me to identify the lines that needed revision.

softwareengineerprogrammer commented 5 months ago

Note from offline discussion with @kfbeckers: We will file a follow-up issue to change the default correlation to intermediate 1 (no action required on @bpulluta's part)