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

Allow having tree stats use direct rank names (and climb tree) #3942

Merged
merged 11 commits into from Nov 22, 2023

Conversation

realVinayak
Copy link
Collaborator

Allows defining a query like 'geography.Country'. Looks for name in the {TREE}treedefitem table.

The previous code allowed just to modify the start values in the last query field, but these changes allow them to be generic.

TO TEST:

  1. Go to stats. All the previous versions of stats should still work - the query would be different in the query builder. New ranks will be visible in the add stats dialog (all ranks above Earth / Life are shown).
  2. The following query should be present (if rank is continent):
    image

Change 2:
Removes distinct from locality stat

@realVinayak
Copy link
Collaborator Author

realVinayak commented Aug 24, 2023

The query that fetches ranks is done only one time. Since tree ranks are fetched outside of stats already, those could also be used instead of this.

But stats like type status utilizes this too (it fetches distinct type status) so I thought it would be simple enough to just have the taxa ones utilize this in the stats spec.

Alternatively, won't be hard to use the tree tank promise outside of stats here instead of fetching them again

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

I might have missed the context behind some of the changes you did as I am not familiar with the code much (and you created a beast with the stats page), but looking good, and I like some of the refactoring you did

@realVinayak realVinayak modified the milestones: 7.9, 7.9.1 Aug 24, 2023
Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

It would take me too long to fully understand how Statistics works to fully consider these changes, but from what I am gathering from this little section, the changes seem fine.

@melton-jason melton-jason requested a review from a team August 25, 2023 14:27
@grantfitzsimmons grantfitzsimmons modified the milestones: 7.9.1, 7.9.2 Sep 26, 2023
@grantfitzsimmons grantfitzsimmons changed the base branch from v7.9-dev to production September 26, 2023 02:27
Copy link

@bronwyncombs bronwyncombs left a comment

Choose a reason for hiding this comment

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

Geog rank stats appeared in the choose stats dialog to be added to a new category. The queries for each rank appear correct. Upon clicking a stat on herb_rbge_9_3_23, the query results were returned promptly, but count was slightly delayed.
Geo represented:
Screenshot 2023-10-25 at 10 11 32 AM
Localities without Distinct:
Screenshot 2023-10-25 at 10 12 05 AM

Also tested on fwri

@grantfitzsimmons
Copy link
Contributor

@realVinayak Seems there are some merge conflicts

@realVinayak
Copy link
Collaborator Author

Also need to test performance on AWS

@realVinayak
Copy link
Collaborator Author

Can this be tested another time by a ux-reviewer before merge? @specify/ux-testing

Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

Looks good!
v7 9-dev-count-indirect-continent

@CarolineDenis CarolineDenis merged commit 9575fc7 into production Nov 22, 2023
9 checks passed
@CarolineDenis CarolineDenis deleted the v7.9-dev-count-indirect branch November 22, 2023 17:44
@specifysoftware
Copy link

This pull request has been mentioned on Specify Community Forum. There might be relevant details there:

https://discourse.specifysoftware.org/t/specify-7-9-2-release-announcement/1452/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants