Set fromtime, modify how we set totime for deactivated (obs_pgm reliance) #182

lcoram · 2025-11-05T20:35:43Z

Fix timeseries fromtime #80
Dealt with by choosing the most restrictive if the timeseries has overlapp with obs_pgm. However there is much discussion about if this is the right way to handle this...?
Deal with the bug where we have from/to times for timeseries that do not overlap at all with the range from obspgm (something I called "twisting") Bug: Totime set to be before fromtime #174
Or do we want to do this manually since once they are "fixed" then we don't need this code?
Actually get the max/min obstime of the underlying data for the timeseries and use that where it makes sense
deactivated = false (do not set to true!)
Or remove this completely from the update query?
Also maybe need to add query that reactivates stuff that was deactivated, or maybe we have to do that manually...?
I have renamed some of the functions, since we are not really deactivating anything...

TODO: do we need to manually set everything that was deactivated=true to false?

lcoram · 2025-11-13T08:40:12Z

ingestion/src/util/stinfosys.rs

+                let _totime = station_totime.get(&label.key.station_id).copied();
+                // NOTE: we are choosing to essentially close off this timeseries, since we believe
+                // it is mislabelled. Obs_pgm is essentially saying it should not exist.
+                Ok((label.id, fromtime, fromtime))


@pmlefeuvre-met @intarga
Think this will do what we discussed yesterday, essentially closing off the timeseries.

Should we comment let _totime = station_totime.get(&label.key.station_id).copied(); as it is eventually not used, it confused me until I saw Ok((label.id, fromtime, fromtime))

Maybe just some semantics, but should the |--station--| be replaced by |--timeseries--| because for me station is another level of information. You can get a fromtime and totime for a station that indicates when a station was opened and closed (from the station table in stinfosys), whereas here we mean a timeseries that is defined by a primarykey/label.met and is defined by stationid, typeid, paramid, sensor, level?

Should we comment let _totime = station_totime.get(&label.key.station_id).copied(); as it is eventually not used, it confused me until I saw Ok((label.id, fromtime, fromtime))

Maybe just some semantics, but should the |--station--| be replaced by |--timeseries--| because for me station is another level of information. You can get a fromtime and totime for a station that indicates when a station was opened and closed, whereas here we mean a timeseries that is defined by a primarykey/label.met and is defined by stationid, typeid, paramid, sensor, level?

-> Relevant for the entire file, is it me who mix things up or station had different definitions in this file @lcoram

This looks like it only handles the case where obs_pgm entirely before station, but not the other way around?

In any case the nest of if conditions is confusing, could maybe be written much more straightforwardly using Timerange::overlap?

Ok, so after discussion with @pmlefeuvre-met I managed to understand that I had been confused about what station_from/totime were, and how they should be used. Now have modified the sql for getting the list of labels so we also actually get the from/to from the DB. This is then used for the first check (to see if its outside the range completely), and after that it defaults to using either obs_pgm if it exists, or station.

Have refactored, hope it is easier to understand and now actually correct.

Is there a reason not to use Timerange::overlap?

Will look at it, sorry... Was just trying to make it make sense first. Get the logic right.

I don't think you understood what I meant. Should it not be possible to get rid of the whole if chain using Timerange::overlap? Since the from/totime you're trying to get out at the end is just the overlap of the ones you're feeding in? Then you just have to handle the None case, unless I missed something?

ingestion/src/util/tsupdate.rs

ingestion/src/util/stinfosys.rs

…unt along with obspgm and station times

ingestion/src/util/stinfosys.rs

…/to update

integration_tests/tests/end_to_end.rs

pmlefeuvre-met · 2025-11-24T10:07:12Z

What is KLOBS? Kvalobs data? Empty data?

…

On Mon, 24 Nov 2025, 10:18 Louise Oram, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In integration_tests/tests/end_to_end.rs <#182 (comment)>: > Some(totime), - // timeseries on station 20001 is not + // no max/min obstime found for KLOBS data? Any ideas why the max/min obstime isn't working for KLOBS? — Reply to this email directly, view it on GitHub <#182 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BDCA5ZGZARSTZLX3NMEB2WL36LENPAVCNFSM6AAAAACLIE6QNCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTIOJZGE4DQMZUGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…for finding max/min obstime

ingestion/src/util/tsupdate.rs

lcoram · 2025-11-27T11:58:40Z

ingestion/src/util/stinfosys.rs

+                    // TODO: set to time to closed based on timeresolution of the timeseries
+                    // for now have to leave it open unless very long time ago???
+                    let ten_years_duration = Duration::days(365 * 10);
+                    let ten_years_ago = Utc::now() - ten_years_duration;


@pmlefeuvre-met @jo-asplin-met-no
It seems that for frost it would be better if we could cut these closer (I mean even 10 years is an improvement over just leaving them open if obs_pgm hasn't closed them)... Right now frost thinks a lot of time series should have data that do not. We talked about cutting these based on time resolution, which we don't technically have yet... :(

I don't think this is the right place to solve this problem. If a timeseries is left open that should be closed, that should be fixed in obs_pgm / stinfosys / eventually content managers. If you want to poke whoever is responsible for closing things in there, you can do a query to make a list for them of things they should probably address.

intarga

I think we could do with some documentation related to this. Some questions that would be good to cover:

What is obs_pgm, and why is it named that?
Why are we using obs_pgm?
Who is responsible for what's in obs_pgm?

intarga · 2025-11-27T16:21:54Z

ingestion/Cargo.toml

 chrono.workspace = true
 chronoutil.workspace = true
 util = { path = "../util" }
+lard_egress = { path = "../egress" }


What is this about? Just to get access the the Timerange type? It should be moved to util, to avoid creating a dependency between ingestion and egress

intarga · 2025-11-27T16:40:50Z

ingestion/src/util/stinfosys.rs

+                    // TODO: set to time to closed based on timeresolution of the timeseries
+                    // for now have to leave it open unless very long time ago???
+                    let ten_years_duration = Duration::days(365 * 10);
+                    let ten_years_ago = Utc::now() - ten_years_duration;


I don't think this is the right place to solve this problem. If a timeseries is left open that should be closed, that should be fixed in obs_pgm / stinfosys / eventually content managers. If you want to poke whoever is responsible for closing things in there, you can do a query to make a list for them of things they should probably address.

intarga · 2025-11-27T18:09:41Z

ingestion/src/util/stinfosys.rs

-pub async fn fetch_deactivated(
-    obs_pgm_totime: &HashMap<MetTimeseriesKey, DateTime<Utc>>,
-    station_totime: &HashMap<i32, DateTime<Utc>>,
+pub async fn fetch_from_to_for_update(


My suggestion to replace this function:

pub fn calc_from_tos( obs_pgm_ranges: &HashMap<MetTimeseriesKey, OpenTimerange>, station_ranges: &HashMap<i32, OpenTimerange>, data_ranges: HashMap<i64, OpenTimerange>, labels: Vec<MetLabel>, ) -> Vec<(i64, OpenTimerange)> { labels.iter().filter_map(|label| { // default is open on both sides let obs_pgm = obs_pgm_ranges.get(&label.key).unwrap_or_default(); let station = station_ranges.get(&label.key.station_id).unwrap_or_default(); // we `?` this one because if it's None, the ts doesn't exist and we can't update it let data = data_ranges.get(&label.id)?; let overlap = obs_pgm.overlap(station).map(|x| x.overlap(data)).flatten(); // if neither of these have a to_time, we should'nt close the ts because it might still // receive new data let should_be_closed = obs_pgm.totime.is_some() || station.totime.is_some(); let out = match (overlap, should_be_closed) { (Some(overlap), true) => overlap, (Some(overlap), false) => OpenTimerange { from: overlap.from, to: None }, (None, true) => OpenTimerange {from: data.to, to: data.to}, (None, false) => OpenTimerange {from: obs_pgm.from.max(station.from).max(data.from), to: None}, }; Some((label.id, out)) }).collect() }

Note that we don't need this to be async or return a result. I think those were originally there because Manuel was querying stinfosys from within this function

The match cases with overlap == None are the "twisted" cases

My comment on should_be_closed is maybe a bit confusing because it's the negative (shouldn't) while the name is positive (shouldn't). Perhaps you can come up with a better comment? Or we can just invert the boolean to let shouldnt_be_closed = obs_pgm.totime.is_none() && station.totime.is_none() and switch the trues and falses in the match?

I like this, the only thing I'm not sure about is that originaly PiM told me to code it so that obs_pgm ranges have precedence over station if the timeseries/station exists in both. Something like

// Prefer obs_pgm if available let stinfo_range = obs_pgm_ranges .get(&label.key) .or(station_ranges.get(&label.key.station_id)) .unwrap_or_default();

But yeah, it's probably worth it to add a comment explaining why, if we want to keep it that way.

intarga · 2025-11-27T18:23:25Z

ingestion/src/util/stinfosys.rs

+                from: Some(fromtime.and_utc()),
+                to: Some(totime.and_utc()),


Are you sure these should be Some? That would mean from/totimes in stinfosys are never NULL. If that's true we can probably use ClosedTimeranges since the ts_fromto is also always closed, and simplify the previous function even more.

Same applies to the station from/to below

That's because the original code only acted on open timeseries in LARD that were supposed to be closed, so in the queries to stinfosys we only select the timeseries that have non-NULL totime

intarga · 2025-11-27T18:25:41Z

ingestion/src/util/tsupdate.rs


-pub struct DeactivatedTimeseries {
+#[derive(Debug)]
+pub struct TSupdateTimeseries {
    /// Timeseries to be updated
    pub tsid: i64,
+    /// Fromtime value found in the metadata source
+    pub fromtime: DateTime<Utc>,
    /// Totime value found in the metadata source
    pub totime: DateTime<Utc>,
 }


As suggested by my function suggestion above, I don't think we need this type, (i64, OpenTimerange) is fine IMO

intarga · 2025-11-27T18:30:11Z

ingestion/src/util/tsupdate.rs

+    Ok(ts_from_to)
+}
+
+pub async fn set_from_to_obs_pgm(


Rename to update_from_to since it's not just based on obs_pgm?

lcoram changed the title ~~add structures to get the obspgm and station fromtimes~~ Set fromtime based on obspgm Nov 5, 2025

lcoram force-pushed the obs_pgm_from_times branch from 38f1102 to f48116d Compare November 5, 2025 20:40

lcoram changed the title ~~Set fromtime based on obspgm~~ Set fromtime , modify how we set totime for deactivated (obs_pgm reliance) Nov 5, 2025

lcoram changed the title ~~Set fromtime , modify how we set totime for deactivated (obs_pgm reliance)~~ Set fromtime, modify how we set totime for deactivated (obs_pgm reliance) Nov 5, 2025

lcoram force-pushed the obs_pgm_from_times branch from 77a59ad to 29917db Compare November 6, 2025 12:34

lcoram marked this pull request as ready for review November 6, 2025 12:37

lcoram requested a review from intarga November 6, 2025 12:37

lcoram force-pushed the obs_pgm_from_times branch from 0034303 to d3e7be4 Compare November 10, 2025 08:27

lcoram commented Nov 13, 2025

View reviewed changes

ingestion/src/util/tsupdate.rs Outdated Show resolved Hide resolved

lcoram marked this pull request as draft November 13, 2025 14:01

intarga reviewed Nov 13, 2025

View reviewed changes

ingestion/src/util/stinfosys.rs Outdated Show resolved Hide resolved

lcoram force-pushed the obs_pgm_from_times branch 2 times, most recently from c580a57 to 65e94c2 Compare November 14, 2025 08:23

lcoram marked this pull request as ready for review November 14, 2025 08:24

lcoram added 13 commits November 14, 2025 21:17

add structures to get the obspgm and station fromtimes

280a609

first pass at logic for setting appropriate from/to for deactivated

5b77f7e

add sql and function for fixing twisted from/to times

2ef868d

refactor to put from / to in better order

278c20f

code to call the untwisting function

4f4c5c0

fix test

5a0849b

set deactivated to false when updating

fae5082

remove untwisting code, but add OR case to open timeseries query

b63c50b

invert from/to in the test

7d3e3ce

close off timeseries that are outside obs_pgm range

2140039

get the from/to for the timeseries from lard, and take that into acco…

b820995

…unt along with obspgm and station times

refactor the if/else logic for deciding which from/to to use

8cd0146

add a note about sql for getting ts from lard

50506d3

lcoram force-pushed the obs_pgm_from_times branch from 65e94c2 to 50506d3 Compare November 14, 2025 20:17

use opentimerange from patchwork

b5b1ac3

Lun4m reviewed Nov 17, 2025

View reviewed changes

ingestion/src/util/stinfosys.rs Outdated Show resolved Hide resolved

simplify code for choosing from/to

db18b0b

Lun4m reviewed Nov 20, 2025

View reviewed changes

ingestion/src/util/stinfosys.rs Outdated Show resolved Hide resolved

get from/to for obs_pgm and station in one call each

2483f41

lcoram force-pushed the obs_pgm_from_times branch from e90c05a to 2483f41 Compare November 20, 2025 09:10

lcoram added 2 commits November 21, 2025 12:56

better naming

e8e3ff4

take into account the max/min obstime of the underlying data for from…

116778e

…/to update

lcoram commented Nov 24, 2025

View reviewed changes

integration_tests/tests/end_to_end.rs Outdated Show resolved Hide resolved

get the paramids that are scalar, for use to make sure use right sql …

ca55dec

…for finding max/min obstime

lcoram commented Nov 26, 2025

View reviewed changes

ingestion/src/util/tsupdate.rs Outdated Show resolved Hide resolved

lcoram commented Nov 27, 2025

View reviewed changes

ingestion/src/util/tsupdate.rs Show resolved Hide resolved

lcoram commented Nov 27, 2025

View reviewed changes

intarga reviewed Nov 27, 2025

View reviewed changes

lcoram added 2 commits December 1, 2025 14:05

ingrids code suggestion, and related cleanup

a2afa20

test on legacy.data, not data and nonscalar_data

7301808

Set fromtime, modify how we set totime for deactivated (obs_pgm reliance) #182

Are you sure you want to change the base?

Set fromtime, modify how we set totime for deactivated (obs_pgm reliance) #182

Uh oh!

Conversation

lcoram commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmlefeuvre-met Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intarga Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmlefeuvre-met commented Nov 24, 2025 via email

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intarga left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lun4m Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lcoram commented Nov 5, 2025 •

edited

Loading

pmlefeuvre-met Nov 13, 2025 •

edited

Loading

intarga Nov 17, 2025 •

edited

Loading

intarga left a comment •

edited

Loading

Lun4m Nov 27, 2025 •

edited

Loading