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

Observations of Names #2499

Open
JoeCohen opened this issue Oct 30, 2024 · 9 comments
Open

Observations of Names #2499

JoeCohen opened this issue Oct 30, 2024 · 9 comments

Comments

@JoeCohen
Copy link
Member

JoeCohen commented Oct 30, 2024

The displayed count for at least the following are broken:

  • other taxa, this taxon proposed
  • any taxon, this name proposed

To replicate:

  • Find a species with Observations, e.g., Lyomyces juniperi (Bourdot & Galzin) Riebesehl & E. Langer
  • Create a new name (without Observations) "Banananana juniperi Test To Be Deleted"
  • Change the synonyms of the first name (Lyomyces juniperi) to include the new Name as a deprecated synonym.
  • Go to the Name page for the new Name.
    Result includes:

other taxa, this taxon proposed (4)
any taxon, this name proposed (4)

@mo-nathan
Copy link
Member

"other taxa, this taxon proposed (2)" is wrong even without adding the synonym since clicking on that link shows no observation. "any taxon, this name proposed (4)" is also wrong since clicking on it only shows 2 observations.

@nimmolo
Copy link
Contributor

nimmolo commented Jan 17, 2025

The issue lies in a discrepancy between

  • the results Query looks up, using the params in app/helpers/names_helper.rb#obss_of_taxon_this_name(name) etc. This generates the results you get via the links, which the UI implies are the "real" results. (They may not be!)
  • the results we calculate in app/models/name/observations.rb by doing a single query for all observations where this name or one of its synonyms was proposed, and selecting subsets of those results for each variant. This technique saves a lot of time loading the show Name page, and was vetted by all of us at the time. It still seems logically solid, and I wouldn't recommend getting rid of this approach, though it needs to be re-examined to be sure it's right.

It's not yet clear which method truly gets the correct results, only that they do not agree. I've just added explanatory notes to the Name::Observations class to help with debugging, via #2660.

@nimmolo
Copy link
Contributor

nimmolo commented Jan 17, 2025

Here is what's going on for each link for Lyomyces juniperi (62566). First is the count given by Name::Observations (in parentheses). Then the count Query is getting, then the SQL that Query is generating:

this name (2)
query.num_results: 2
query.last_query: SELECT DISTINCT observations.id FROM observations WHERE observations.name_id IN (62566)

this taxon, other names (2)
query.num_results: 2
query.last_query: SELECT DISTINCT observations.id FROM observations WHERE observations.name_id IN (29756)

this taxon, any name (4)
query.num_results: 4
query.last_query: SELECT DISTINCT observations.id FROM observations WHERE observations.name_id IN (29756,62566)

other taxa, this taxon proposed (0)
query.num_results: 0
query.last_query: SELECT DISTINCT observations.id FROM observations JOIN namings ON namings.observation_id = observations.id WHERE namings.name_id IN (29756,62566) AND observations.name_id NOT IN (29756,62566)

any taxon, this name proposed (2)
query.num_results: 2
query.last_query: SELECT DISTINCT observations.id FROM observations JOIN namings ON namings.observation_id = observations.id WHERE namings.name_id IN (62566)

subtaxa (525509)
?? This is simply counting Observation.all because the @subtaxa_query is doing the wrong(?) filters.
There's an issue in NamesController#init_related_query_ivars.
@children_query (for the name) is returning all names.
Here are its params:
@params={:names=>["62566"], :include_immediate_subtaxa=>true, :exclude_original_names=>true}
This generates:
SELECT DISTINCT names.id FROM names WHERE names.correct_spelling_id IS NULL

@nimmolo
Copy link
Contributor

nimmolo commented Jan 17, 2025

In the example given for Name 62566, in my db snapshot, the query seems to be in sync with the subset methods in Names::Observations. So I'm focusing on the query of subtaxa now, which is the farthest off base — both for "Observations of subtaxa", and the query in the Classification box that links to the list of subtaxa (names).

I went back in blame on GitHub before my entire Query refactor, and I do not seem to have altered any of the relevant Query code in query/initializers/names.rb and query/modules/lookup_names.rb, although I have certainly moved it around. And I don't know how long the subtaxa issue has been there. @JoeCohen or @mo-nathan do you remember anything about how long subtaxa specifically has been wrong? I haven't been paying attention to this page since I worked on it.

I did a few hours debugging last night. The query that returns the wrong results is this one, in NamesController#show and friends:

@children_query = create_query(:Name,
                               names: @name.id,
                               include_immediate_subtaxa: true,
                               exclude_original_names: true)

Seems the params are correctly registered by Query. It would be very surprising if they weren't!

@params={:names=>["62566"], :include_immediate_subtaxa=>true, :exclude_original_names=>true}

But that query generates the following SQL - an unfiltered index of all names:

SELECT DISTINCT [names.id](http://names.id/) FROM names WHERE names.correct_spelling_id IS NULL

I checked out Query::Modules::LookupNames and specifically how it’s handling exclude_original_names For a name with no children , if we include_immediate_subtaxa but do not exclude_original_names, the query produces an array containing the id of the original name itself, which is as expected, I believe. If both params are true, in the case of Lyomyces juniperi (62566) it correctly produces an empty array of ids — i.e. this name has no subtaxa, also as expected. So the issue seems not to be in LookupNames.

I suspect it's in Query::Initializers::Names. I imagine what's going wrong is something simple, like the query should return empty results if it doesn’t get any ids in the array, but it’s returning unfiltered results instead (as a scope would). Query::Initializers::Names has a method force_empty_results which is used in other cases, but not the case of an empty ids array.

@JoeCohen Jason thinks you may have added the exclude_original_names param, just to jog your memory. But blame says that was more than 5 years ago.

@JoeCohen
Copy link
Member Author

  1. I have no memory of adding that. But it's possible. I no longer remember how all this stuff works.
    I did add exclude_original_names to app/helpers/names_helper.rb#obss_of_taxon_other_names

  2. The "observations of subtaxa" line has been broken for a while. (At least a month, but I think more.)

  3. But it seems to show the correct number when the Name actually has subtaxa. (BTW MO treats a Group as a subtaxon of Species.)

@nimmolo
Copy link
Contributor

nimmolo commented Jan 18, 2025

Thanks @JoeCohen. I may be able to handle the subtaxa thing, then.

If you could write a failing test that reproduces the error you first described in this issue, it would help figure out whether this is some kind of caching issue (which would only show up in a system test) or a deeper issue with Query.

@JoeCohen
Copy link
Member Author

JoeCohen commented Jan 18, 2025 via email

@JoeCohen
Copy link
Member Author

Upon review:

  1. The original bug report was off.
  2. It is correct as amended by @mo-nathan's amended bug report.

Observations of:
this name (2)
this taxon, other names (2)
this taxon, any name (4)
other taxa, this taxon proposed (2)
any taxon, this name proposed (4)
subtaxa (527992)

  • 'The links for "other taxa, this taxon proposed (2)" is wrong even without adding the synonym since clicking on that link shows no observation."'
    -- The displayed number is wrong -- it should be 0.
    --The link is correct (there are no other taxa for which Lyomyces juniperi was proposed.
  • '"any taxon, this name proposed (4)" is also wrong since clicking on it only shows 2 observations.'
    -- The displayed number (4) is correct.
    -- The link is wrong. (It should display 4 obss.)

@JoeCohen
Copy link
Member Author

JoeCohen commented Jan 18, 2025 via email

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

No branches or pull requests

3 participants