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

Support time window query #6872

Merged
merged 23 commits into from
Sep 14, 2023
Merged

Support time window query #6872

merged 23 commits into from
Sep 14, 2023

Conversation

mwu2018
Copy link
Contributor

@mwu2018 mwu2018 commented Sep 5, 2023

What this PR does

Fixes #6873
Support time window query for esri-mapServer type.

  • Add timeWindowDuration and timeWindowUnit traits to type esri-mapServer to support time query with window.
  • A positive timeWindowDuration indicate forward time window while negative value backward window.
  • The timeWindowUnit can be any units recognised by moment module. If an invalid value is given, will query time without window.

Test me

before

after

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@mwu2018 mwu2018 marked this pull request as ready for review September 6, 2023 01:09
Copy link
Collaborator

@na9da na9da left a comment

Choose a reason for hiding this comment

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

@mwu2018 - I have partially reviewed the PR and think it is a great start and good work on the specs.

In the context of ArcGis MapServer, intervals actually mean the time step between two dates in the date selector (or the timeline). Whereas the concept we are implementing here - the window of data rendered on the map - is called a time window. So ideally we should rename the traits to something that aligns with the original terms.

Another thing we could do is combine the units and time window setting into one. We already use momentjs in a few places so you could use that to parse a string like 10s or 1M.

If we do that you could name the trait as timeWindowDuration and accept strings like 1M 1Y. The direction trait could be named as timeWindowDirection.

Let me know what you think.

lib/Models/Catalog/Esri/ArcGisMapServerCatalogItem.ts Outdated Show resolved Hide resolved
lib/Traits/TraitsClasses/TimeVaryingTraits.ts Outdated Show resolved Hide resolved
@mwu2018
Copy link
Contributor Author

mwu2018 commented Sep 8, 2023

momentjs

The doc says momentjs is not recommended for new projects.

@mwu2018
Copy link
Contributor Author

mwu2018 commented Sep 8, 2023

@na9da I've revised partially based on your comments. Happy to discuss.

@na9da
Copy link
Collaborator

na9da commented Sep 11, 2023

The doc says momentjs is not recommended for new projects.

That is correct, though the project hasn't been deprecated and we already use it in terriajs.

I think it should be ok to use it for now until we switch to something else - #3350

@na9da na9da self-requested a review September 11, 2023 01:27
@mwu2018 mwu2018 changed the title Support time interval Support time window query Sep 11, 2023
Comment on lines 412 to 429
private getTimeParams(): TimeParams {
const currentTime = this.getCurrentTime();
const timeWindowDuration = this.timeWindowDuration;
const timeWindowUnit = this.timeWindowUnit;
const isForwardTimeWindow = this.isForwardTimeWindow;
const timeParams = {
currentTime,
timeWindowDuration,
timeWindowUnit,
isForwardTimeWindow
} as TimeParams;
return timeParams;
}

const imageryProvider = this._createImageryProvider(dateAsUnix);
@computed
private get _currentImageryParts(): ImageryParts | undefined {
const timeParams = this.getTimeParams();
const imageryProvider = this._createImageryProvider(timeParams);
Copy link
Collaborator

@na9da na9da Sep 14, 2023

Choose a reason for hiding this comment

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

The getTimeParams() needs to be a @computed. This is because we are passing it as parameter to _createImageryProvider which itself is a computed (memoized) object. But here, getTimeParams returns a new object each time which will result in re-computing of _createImageryProvider. This is an expensive operation. You can avoid this by converting it to something like @computed get timeParams() {...}.

To see this in action, open the network tab for both the before and after links and try changing the opacity. For the after link, see that changing the opacity results in network fetches, whereas the network tab is silent when doing this for the before link.

Copy link
Collaborator

@na9da na9da left a comment

Choose a reason for hiding this comment

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

Great work @mwu2018 - looks good to go 🚀

@mwu2018 mwu2018 merged commit 881b779 into main Sep 14, 2023
5 checks passed
@mwu2018 mwu2018 deleted the support-time-interval branch September 14, 2023 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support time window query
3 participants