261-erroneous-values-in-database-for-moisture-variables #285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…and windV and formatting by RUFF
|
formatted and pushed two files that needed formatting |
ian-noaa
reviewed
Dec 28, 2023
mats_metadata_and_indexes/index_creation_scripts/get_index_definitions.sh
Outdated
Show resolved
Hide resolved
|
I think I'd rather just go ahead and fix the remains in the new work. Since
the tests are all currently passing and this won't really affect anything.
…On Thu, Dec 28, 2023 at 2:31 PM Ian McGinnis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In mats_metadata_and_indexes/index_creation_scripts/README.md
<#285 (comment)>:
> - mats_metadata_and_indexes/index_creation_scripts/create_indexes-0-replicas.n1ql
- This is for the standalone server since it cannot support index replication (only one node)
+ This is for the standalone server since it cannot support index replication (it only has one node)
Copy the contents of this N1QL script and either execute it in the query window of the UI of the
standalone server or execute it with the command line interface cbq.
After that you must also execute the N1QL in the build_indexes.n1ql script.
Okay. They definitely should be updated but I'm okay with doing it in
either branch. If all else is equal, I like coupling documentation updates
in the same branch as the code changes so they don't get forgotten.
If you wanted to get really fancy, you could also remove these changes
from this branch/PR as well so they'd all happen in the right feature
branch. That'd be some moderate git surgery though.
—
Reply to this email directly, view it on GitHub
<#285 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGDVQPRI7W6LPE2BQ5OEZDLYLXQMHAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJYGQZDIOBQGU>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
--
Randy Pierce
|
|
Really big concern - but I think the consensus (Jeff and Molly) is that it
is a really big effort and we need to get this working by the first, so
since the legacy is this way anyway we can address it all in one thing at a
later date. The app is currently using it with mixed units.
…On Thu, Dec 28, 2023 at 2:34 PM Ian McGinnis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/vxingest/netcdf_to_cb/netcdf_builder.py
<#285 (comment)>:
> + relative_humidity_from_dewpoint(
+ temperature * units.kelvin, dewpoint * units.kelvin
+ ).magnitude
+ ) * 100
Okay. Is there any concern about inconsistent units in the database? I
suspect we're in that situation already. However, I mostly want to make
sure that doing the backfilling in Celsius and the realtime ingest in
Kelvin won't cause issues.
—
Reply to this email directly, view it on GitHub
<#285 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGDVQPR2EMMEBZQBK35JW2DYLXQVPAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJYGQZDMNJUGI>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
--
Randy Pierce
|
|
Hold up, we don’t currently have mixed units in the database? Everything is
in Fahrenheit, although we want to change that to Celsius. Most models
output in kelvin, and most obs in Celsius or Fahrenheit, but we definitely
standardize all of that before it’s stored in the database, yes?
On Thu, Dec 28, 2023 at 2:38 PM randytpierce ***@***.***>
wrote:
… Really big concern - but I think the consensus (Jeff and Molly) is that it
is a really big effort and we need to get this working by the first, so
since the legacy is this way anyway we can address it all in one thing at
a
later date. The app is currently using it with mixed units.
On Thu, Dec 28, 2023 at 2:34 PM Ian McGinnis ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/vxingest/netcdf_to_cb/netcdf_builder.py
> <#285 (comment)>:
>
> > + relative_humidity_from_dewpoint(
> + temperature * units.kelvin, dewpoint * units.kelvin
> + ).magnitude
> + ) * 100
>
> Okay. Is there any concern about inconsistent units in the database? I
> suspect we're in that situation already. However, I mostly want to make
> sure that doing the backfilling in Celsius and the realtime ingest in
> Kelvin won't cause issues.
>
> —
> Reply to this email directly, view it on GitHub
> <#285 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AGDVQPR2EMMEBZQBK35JW2DYLXQVPAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJYGQZDMNJUGI>
> .
> You are receiving this because you were assigned.Message ID:
> ***@***.***>
>
--
Randy Pierce
—
Reply to this email directly, view it on GitHub
<#285 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHBWGZNNLNC54KKSPNPDBP3YLXREPAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZRGUYDKMZQGU>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
I am trying to make the models and obs match right now, with the back
filling, whatever they are. We can change them all at once in the future.
That is my intent. I'll double check.
randy
On Thu, Dec 28, 2023 at 2:50 PM Molly Smith ***@***.***>
wrote:
… Hold up, we don’t currently have mixed units in the database? Everything
is
in Fahrenheit, although we want to change that to Celsius. Most models
output in kelvin, and most obs in Celsius or Fahrenheit, but we definitely
standardize all of that before it’s stored in the database, yes?
On Thu, Dec 28, 2023 at 2:38 PM randytpierce ***@***.***>
wrote:
> Really big concern - but I think the consensus (Jeff and Molly) is that
it
> is a really big effort and we need to get this working by the first, so
> since the legacy is this way anyway we can address it all in one thing
at
> a
> later date. The app is currently using it with mixed units.
>
> On Thu, Dec 28, 2023 at 2:34 PM Ian McGinnis ***@***.***>
> wrote:
>
> > ***@***.**** commented on this pull request.
> > ------------------------------
> >
> > In src/vxingest/netcdf_to_cb/netcdf_builder.py
> > <#285 (comment)>:
> >
> > > + relative_humidity_from_dewpoint(
> > + temperature * units.kelvin, dewpoint * units.kelvin
> > + ).magnitude
> > + ) * 100
> >
> > Okay. Is there any concern about inconsistent units in the database? I
> > suspect we're in that situation already. However, I mostly want to
make
> > sure that doing the backfilling in Celsius and the realtime ingest in
> > Kelvin won't cause issues.
> >
> > —
> > Reply to this email directly, view it on GitHub
> > <#285 (comment)>,
> > or unsubscribe
> > <
>
https://github.com/notifications/unsubscribe-auth/AGDVQPR2EMMEBZQBK35JW2DYLXQVPAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJYGQZDMNJUGI>
>
> > .
> > You are receiving this because you were assigned.Message ID:
> > ***@***.***>
> >
>
>
> --
> Randy Pierce
>
> —
> Reply to this email directly, view it on GitHub
> <#285 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AHBWGZNNLNC54KKSPNPDBP3YLXREPAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZRGUYDKMZQGU>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#285 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGDVQPR2PMXUKFRWCC5JWM3YLXSSJAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZRGUYTAOJZGA>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
--
Randy Pierce
|
|
Yes, that sounds good.
On Thu, Dec 28, 2023 at 2:55 PM randytpierce ***@***.***>
wrote:
… I am trying to make the models and obs match right now, with the back
filling, whatever they are. We can change them all at once in the future.
That is my intent. I'll double check.
randy
On Thu, Dec 28, 2023 at 2:50 PM Molly Smith ***@***.***>
wrote:
> Hold up, we don’t currently have mixed units in the database? Everything
> is
> in Fahrenheit, although we want to change that to Celsius. Most models
> output in kelvin, and most obs in Celsius or Fahrenheit, but we
definitely
> standardize all of that before it’s stored in the database, yes?
>
>
> On Thu, Dec 28, 2023 at 2:38 PM randytpierce ***@***.***>
> wrote:
>
> > Really big concern - but I think the consensus (Jeff and Molly) is
that
> it
> > is a really big effort and we need to get this working by the first,
so
> > since the legacy is this way anyway we can address it all in one thing
> at
> > a
> > later date. The app is currently using it with mixed units.
> >
> > On Thu, Dec 28, 2023 at 2:34 PM Ian McGinnis ***@***.***>
> > wrote:
> >
> > > ***@***.**** commented on this pull request.
> > > ------------------------------
> > >
> > > In src/vxingest/netcdf_to_cb/netcdf_builder.py
> > > <
#285 (comment)>:
>
> > >
> > > > + relative_humidity_from_dewpoint(
> > > + temperature * units.kelvin, dewpoint * units.kelvin
> > > + ).magnitude
> > > + ) * 100
> > >
> > > Okay. Is there any concern about inconsistent units in the database?
I
> > > suspect we're in that situation already. However, I mostly want to
> make
> > > sure that doing the backfilling in Celsius and the realtime ingest
in
> > > Kelvin won't cause issues.
> > >
> > > —
> > > Reply to this email directly, view it on GitHub
> > > <
#285 (comment)>,
>
> > > or unsubscribe
> > > <
> >
>
https://github.com/notifications/unsubscribe-auth/AGDVQPR2EMMEBZQBK35JW2DYLXQVPAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJYGQZDMNJUGI>
>
> >
> > > .
> > > You are receiving this because you were assigned.Message ID:
> > > ***@***.***>
> > >
> >
> >
> > --
> > Randy Pierce
> >
> > —
> > Reply to this email directly, view it on GitHub
> > <#285 (comment)>,
>
> > or unsubscribe
> > <
>
https://github.com/notifications/unsubscribe-auth/AHBWGZNNLNC54KKSPNPDBP3YLXREPAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZRGUYDKMZQGU>
>
> > .
> > You are receiving this because you are subscribed to this
thread.Message
> > ID: ***@***.***>
> >
>
> —
> Reply to this email directly, view it on GitHub
> <#285 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AGDVQPR2PMXUKFRWCC5JWM3YLXSSJAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZRGUYTAOJZGA>
> .
> You are receiving this because you were assigned.Message ID:
> ***@***.***>
>
--
Randy Pierce
—
Reply to this email directly, view it on GitHub
<#285 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHBWGZPEKAPMMAC4Z3OLLC3YLXTFPAVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZRGUYTGMZXHA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
ian-noaa
approved these changes
Dec 28, 2023
There was a problem hiding this comment.
Looks good with the understanding that:
- the README sections about the
*.n1qlscripts deleted in this PR will be updated in a follow-on PR - we don't have concerns about the differing Kelvin & Celsius inputs for the
relative_humidity_from_dewpointcalcuation in the NetCDF builder & the backfiling, respectively.
|
Yes, this is the conclusion I came to. Should I just correct the backfil l
script in the 261 branch or would you rather merge that and then make a new
branch? I am thinking just do it in the 261 branch and existing PR.
Randy Pierce
…On Fri, Dec 29, 2023 at 10:57 AM Ian McGinnis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/vxingest/netcdf_to_cb/netcdf_builder.py
<#285 (comment)>:
> + relative_humidity_from_dewpoint(
+ temperature * units.kelvin, dewpoint * units.kelvin
+ ).magnitude
+ ) * 100
After some offline conversation, it turns out that doing the realtime
ingest in Kelvin is correct - the values in these particular (maybe all?)
NetCDF files are in degrees Kelvin. However, the backfilling script should
have been using degrees Fahrenheit instead of Celsius. The backfilling
script operates on the values in the database and those values are stored
in degrees Fahrenheit.
Ideally, in the near future, the raw data files will have whatever units
make sense (most likely Kelvin) and the database will store everything in
metric.
—
Reply to this email directly, view it on GitHub
<#285 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGDVQPX2TH7ZEOMJEUNP6FDYL4AB3AVCNFSM6AAAAABBFX37KWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJZGE2DGNJSGU>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
|
I think making the fix to the backfilling script in this PR makes a ton of sense to me. Then we can merge this PR once the script is fixed. |
randytpierce
deleted the
261-erroneous-values-in-database-for-moisture-variables
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR should add changes to the netcdf builder to accomodate RH, WindU, and WindV values and also a backfill scripot, along with some other minor changes.