OpenTreeOfLife / germinator

miscellaneous scripts and data for concerns that span more than one of the Open Tree code repositories: integration tests, system statistics, etc.
BSD 2-Clause "Simplified" License
21 stars 7 forks source link

Expose height_limit parameter for newick? (subtree method) #55

Closed jar398 closed 8 years ago

jar398 commented 8 years ago

height_limit is implemented for newick, but the spec says it isn't. I had not expected this feature to get implemented, and it is not necessary for the v3 release, but as it is there and it has some utility, I think we should consider it.

This issue is split off from #54.

I think it's probably a good idea, but I wonder if the design is complete. For example, how does a client distinguish tips in the returned tree that are tips in the synthetic tree, from tips that are tips only because the height limit was reached?

One option is to delay discussion. I'm content with leaving it out now and maybe adding it to a v3 point release.

josephwb commented 8 years ago

It doesn't say not for newick It says:

height_limit : integer e.g. 3 -- for arguson

Label format, on the other hand, is explicit:

label_format: string for newick only

josephwb commented 8 years ago

From api doc:

height_limit could be applicable to newick, if we get around to implementing height-limited newick in time; this would be preferred since it's cleaner and has lower documentation burden.

mtholder commented 8 years ago

If this feature is working, then it makes sense to me to include it. We could add an additional field to the response to list the tips that were the slice points in the event that the height limit is triggered. Even without that info, a client could use this service (because they could check for the tips returned in the taxonomy).

josephwb commented 8 years ago

It seems like "feature is working" has no bearing on things. But I agree with your other points.

kcranston commented 8 years ago

Couple of questions:

josephwb commented 8 years ago

Default for arguson is 5. Default for newick is -1 (that is, no limit).

josephwb commented 8 years ago

Here is an example call currently deployed on test (don't worry: newickDepth is temporary):

curl -X POST https://test.opentreeoflife.org/v3/tree_of_life/subtree -H "content-type:application/json" -d '{node_id":"ott81461", "label_format":"name_and_id", "height_limit":3}'
{
  "newickDepth" : 3,
  "newick" : "(((Struthionidae_ott857849)Struthioniformes_ott857847,(Rheiformes_ott829553,mrcaott165688ott167137)mrcaott165688ott857860)Palaeognathae_ott81443,((Crecopsis_egregia_ott5857242)Crecopsis_ott5852537,(Nesophylax_ater_ott5857241)Nesophylax_ott5852536,(Edithornis_sylvestris_ott5560002)Edithornis_ott5559003,(Cabalus_modestus_ott5342864)Cabalus_ott5342841,Palaeorallus_ott4131104,Parvirallus_ott4131100,Latipons_ott4131091,Eocrex_ott4131074,(Dryolimnas_cuvieri_ott3600005)Dryolimnas_ott3599993,(Aenigmatolimnas_marginalis_ott3600004)Aenigmatolimnas_ott3599979,(Cyanolimnas_cerverai_ott3599999)Cyanolimnas_ott3600000,(Diaphorapteryx_hawkinsi_ott3599989)Diaphorapteryx_ott3599978,(Atlantisia_podarces_ott3599991,Atlantisia_rogersi_ott3599985)Atlantisia_ott3599975,(Neocrex_colombianus_ott5857240,Neocrex_colombiana_ott3599995,Neocrex_erythrops_ott3599983)Neocrex_ott3599984,(Rougetius_rougetii_ott3599981)Rougetius_ott3599982,(Gymnocrex_rosenbergii_ott3600007,Gymnocrex_talaudensis_ott3600006,Gymnocrex_plumbeiventris_ott3599980,Gymnocrex_plumbiventris_ott3599976)Gymnocrex_ott3599977,(Aphanapteryx_leguati_ott3599997,Aphanapteryx_bonasia_ott3599973)Aphanapteryx_ott3599974,Eurostopodus_guttatus_ott3597050,Eurostopodus_archboldi_ott3597049,Eurostopodus_temminckii_ott3597048,Eurostopodus_diabolicus_ott3597047,(Lewinia_mirificus_ott5857219,Lewinia_muelleri_ott3599992,Lewinia_mirifica_ott3599970,Lewinia_pectoralis_ott978545)Lewinia_ott565044,(Nesoclopeus_poecilopterus_ott3600008,Nesoclopeus_woodfordi_ott872389)Nesoclopeus_ott872388,(Micropygia_schomburgkii_ott971968)Micropygia_ott971963,(Gallicrex_cinerea_ott943596)Gallicrex_ott943595,(Crex_egregia_ott3599896,Crex_crex_ott897460)Crex_ott897464,(Megacrex_inepta_ott553404)Megacrex_ott553403,(Pardirallus_nigricans_ott3599904,Pardirallus_maculatus_ott1024692,Pardirallus_sanguinolentus_ott490362)Pardirallus_ott490361,(Canirallus_oculeus_ott3599968,Canirallus_beankaensis_ott714658,Canirallus_kioloides_ott985244)Canirallus_ott985261,(Cladorhynchus_ott703809,Recurvirostra_ott917050,Himantopus_ott274627)Recurvirostridae_ott703807,(Coturnicops_notatus_ott3599917,Coturnicops_exquisitus_ott556660,Coturnicops_noveboracensis_ott242758)Coturnicops_ott242761,(Stercorarius_ott742632)Stercorariidae_ott168297,(Rallina_leucospila_ott3599903,Rallina_tricolor_ott3599902,Rallina_forbesi_ott3599901,Rallina_canningi_ott3599900,Rallina_fasciata_ott3599899,Rallina_rubra_ott3599898,Rallina_mayri_ott3599897,Rallina_eurizonoides_ott765966)Rallina_ott765965,(Habroptila_wallacei_ott3599972,Habroptila_wallacii_ott174014)Habroptila_ott70699,(Mancalla_ott4130471,Miocepphus_ott4130462,mrcaott385981ott855480,Cyclorrhynchus_ott280906,mrcaott147723ott280897)Alcidae_ott855478,(Aramides_wolfi_ott3599926,Aramides_saracura_ott3599925,Aramides_mangle_ott3599924,Aramides_calopterus_ott3599923,Aramides_cajaneus_ott3599922,Aramides_cajanea_ott906602,Aramides_ypecaha_ott228505,Aramides_axillaris_ott101222)Aramides_ott381374,(Amaurolimnas_concolor_ott73837)Amaurolimnas_ott73839,(Aramidopsis_plateni_ott70721)Aramidopsis_ott238069,(Amaurornis_bicolor_ott5857218,Amaurornis_isabellinus_ott5857217,Limnocorax_flavirostra_ott5857216,Amaurornis_moluccana_ott5560000,Amaurornis_phoenicurus_ott318444,Amaurornis_flavirostra_ott3599921,Amaurornis_isabellina_ott3599920,Amaurornis_magnirostris_ott3599918,Amaurornis_akool_ott979369,Amaurornis_olivieri_ott74389,Amaurornis_olivacea_ott70703)Amaurornis_ott380188,(Eulabeornis_tricolor_ott3599971,Eulabeornis_castaneoventris_ott70700)Eulabeornis_ott237579,(Smutsornis_ott4130524,Pluvianus_ott242773,Cursorius_ott214788,Glareola_ott385959,Stiltia_ott129397,Rhinoptilus_ott57832)Glareolidae_ott980445,(Chionis_ott738519)Chionididae_ott738518,(Anurolimnas_fasciatus_ott3599936,Anurolimnas_viridis_ott595231,Anurolimnas_castaneiceps_ott32324)Anurolimnas_ott32327,(Carbo_ott3600673,Mesocarbo_ott3600670,Microcarbo_ott3600669,mrcaott9830ott480885)Phalacrocoracidae_ott969838,(Anseriformes_ott241841,Galliformes_ott837585)Galloanserae_ott5839486,(mrcaott5481ott17146,mrcaott246ott5021)mrcaott246ott5481)Neognathae_ott241846)Aves_ott81461;"
}
jar398 commented 8 years ago

Where the spec says "-- for arguson" that implies, in a Gricean way, that it is not for newick. That is, why would it say "for arguson" if it were intended to also apply to newick? So we are discussing a spec change here, and at this point in the process there should be a heavy bias against any spec change.

That said, I'm in favor of changing the spec so that the height_limit is applicable to all output formats. (I would probably not be in favor if it were up to me to make the implementation conform to the spec change, or if someone had produced another argument against.)

I suspect we've implicitly adopted the change, so this may be moot, but the issue needs to be closed out somehow.

Re @kcranston's questions:

kcranston commented 8 years ago

I think this is now done, so closing.