bradymiller wrote on Wednesday, February 06, 2013:
btw,
The testing scripts are a nice addition also,
-brady
yehster wrote on Wednesday, February 06, 2013:
https://github.com/yehster/openemr/commit/2345cbbf0331ac35808d2bc98f69f464a3fd17c5
This fixes “description” mode search to allow empty string and added a test case.
(odd that the error only happens if there are no flagged ‘Diagnosis Reporting Only’ and/or ‘Service Reporting Only’ items)
Not really that odd. If there are flagged rows, then the “code” mode of sequential search will have results and the function returned before trying an empty description search, which was the core of the issue.
yehster wrote on Wednesday, February 06, 2013:
In case you are interested, this is the count of active concepts in my particular SNOMED tables.
(disorder) 170437
(procedure) 126609
(organism) 86875
(finding) 83195
(body structure) 74377
(substance) 62077
(product) 42092
(qualifier value) 22585
(observable entity) 21130
(morphologic abnormality) 13249
(physical object) 11133
(situation) 8415
(occupation) 8169
(event) 8134
(regime/therapy) 6015
(specimen) 2948
(assessment scale) 2687
(environment) 2538
(attribute) 2414
(cell) 1695
(navigational concept) 1517
(geographic location) 1348
(cell structure) 1198
(person) 1000
(tumor staging) 754
(ethnic group) 537
(religion/philosophy) 502
(record artifact) 493
(physical force) 440
(namespace concept) 355
(social concept) 67
(racial group) 49
(life style) 44
(staging scale) 36
(link assertion) 17
(inactive concept) 16
(SNOMED RT+CTV3) 5
(environment / location) 3
(linkage concept) 2
(special concept) 2
bradymiller wrote on Wednesday, February 06, 2013:
Hi,
Very curious to see what the performance of the SNOMED search will be in your fee sheet real-time search; if it’s dismal, may start figuring out a way to “optimize” the snomed lookups.
-brady
OpenEMR
bradymiller wrote on Wednesday, February 06, 2013:
hi,
Looks like your way ahead of me; just noticed your precompute branch. Doing something like that is definitely feasible. It could be part of the SNOMED import script and could be placed in the openemr upgrade script.
-brady
OpenEMR
yehster wrote on Wednesday, February 06, 2013:
Each sequential_code_set_search request from the fee sheet takes a little more than 1 second on my system. A request only happens when the user pauses typing for 400 milliseconds. So it’s not really “real-time” It feels a lot more sluggish than the ICD9/ICD10 searches which return in 10-20 milliseconds for me.
Precomputing the join speeds things up by a factor of 2.
I can get another factor of 2 improvement by the interesting optimization of indexing ConceptId in the precomputed table but also switching the data type to CHAR(20) instead of BIGINT(20).
The reason why the switch to CHAR(20) seems to be necessary is that “like 123%” can take advantage of the index when it’s a string instead of a number. The index sorts then like 1,10,11,12,20,21,22,30,3,30,4,40… etc… when it’s strings instead of by value order.
Anyway, except for the performance issues, what do you think of the overall approach with the metadata?
bradymiller wrote on Wednesday, February 06, 2013:
Hi Kevin,
The metadata approach is simply awesome. It probably makes sense to commit your code-types-language branch to the master now since everything is testing nicely, which also brings in all the related components(Spanish and use of the sequential algorithm throughout codebase). In the code_types.php script, guessing some comments to clean up and maybe migrating the define_external_table() to the function section, but is all up to you.
Regarding the optimization, if your fee sheet search is acceptable in SNOMED, then this could always be attacked at a later date (if needed) after things settle out for a bit and the code gets tested by international users (unless you want to take it on now). I’m happy to help out with the import/upgrade components, if and when needed. Sounds like performance improvements in order of importance would be (note the ones at bottom probably won’t improve things to much):
1. New table that basically duplicates sct_description with a column for the semantic tag that is indexed (the only trade-off I can think of is that we have added an additional large amount of data for the backup process; I suppose an alternative would be to add the columns directly to the sct_descriptions table; then may want to even make an additional ConceptIdChar column for the indexed column to ensure the standard tables will be compatible with any standard snomed tools that are brought into OpenEMR in the future)
2. Index ConceptId and change it to a CHAR (note the simple indexing of this column should make the lookup_code_descriptions() super fast and very cool to learn that it will even get used in the $var.% search)
3. Index DescriptionType (should decrease the rows scanned by a factor of 3)
4. Index DescriptionStatus (should also decrease the rows scanned)
Again, seems perfectly appropriate to delay the performance optimization (unless you want to take it on now). If we delay, then I’ll add this to the Active Projects page, so this thread, plan and your code isn’t forgotten:
http://open-emr.org/wiki/index.php/Active_Projects
-brady
OpenEMR
yehster wrote on Wednesday, February 06, 2013:
I have pushed our combined branch into sourceforge.
The impact of “3” (DescriptionType) index may not be significant. I think you observed something similar before. It’s worth looking at.
#4 (Index Description status) is taken care of in my precomputed table as I only bring in “DescriptionStatus=0” to begin with.
bradymiller wrote on Wednesday, February 06, 2013:
Hi,
Thanks for the valued contribution.
So, could also potentially only bring in DescriptionType 1 and 3 into the precomputed table (since 2, which are synonyms ,is not used) and then an index on it would cut the scan in half (again, the practical impact may be negligible, though). Something to keep in mind is that the Adminitration->Services script can actually do the query 4 times (if goes to description) since it does a count first(ie. the performance difference is amplified). So, that’s a good place to judge “practical” speed.
-brady
OpenEMR
bradymiller wrote on Wednesday, February 06, 2013:
I think I just remembered why I put a count parameter in those search functions rather than count it after the call. I figured at some point that it may be more efficient to work the count into the query itself.
-brady
bradymiller wrote on Friday, February 08, 2013:
Hi Kevin (and everybody else),
The Administration->Services script was bogging down and freezing when working on the SNOMED codes. It did not appear to be a search performance issue, but more a resource issue; because it would work fine for first couple searches, but would then get slower for each query until sometimes hanging altogether. I suspected it was the fact that the count method in code_set_search() simply collects the entire query and then counts it after the fact.
So, I fixed this by incorporating the counting mechanism within the actual queries, which made things much better in the Administration->Services screen. Note that in the process of doing this, I did a couple other things (see the commit for the details of each item):
-Incorporated count mechanism into the query in the code_set_search() function.
-Templating the common_columns variable a bit more in code_set_search() function
-Made the sequential_code_set_search() more compact and abstract to support additional searching methods (and ordering) in the future.
-Added some more test function (and revised the main function call there) calls in SequentialTests.php scripts.
One thing that is not very related, but I noted in the SequentialTests.php script was that the sqlFetchArray() wrapper was not being used to cycle through the query in the SequentialTests.php, so I also changed this. Kevin, is there a benefit to changing this openemr standard? I generally recommend only using the sqlFetchArray() in the openemr codebase because it supports both resource and record set and provides a central location in case any “codebase-wide” changes are required in the future.
Please let me know your thoughts on the commit. Still got some minimal testing left to do, but it is ready for review.
thanks,
-brady
OpenEMR
bradymiller wrote on Friday, February 08, 2013:
Here is the commit (forgot to post it above):
http://github.com/bradymiller/openemr/commit/a6918ed3ae2a3c91c9e6baf9573de08293b916d9
-brady
yehster wrote on Friday, February 08, 2013:
Brady,
What you implemented is exactly what I imagined you meant a couple of comments ago.
I think I just remembered why I put a count parameter in those search functions rather than count it after the call. I figured at some point that it may be more efficient to work the count into the query itself.
Makes sense.
Everything looks good to me.
bradymiller wrote on Friday, February 08, 2013:
Hi,
The above commit is testing well, so committed it to sourceforge.
-brady
OpenEMR
bradymiller wrote on Saturday, February 09, 2013:
Hi,
Noted a bug in Administration->Services, where the ALL setting is also showing items that shouldn’t be shown. For example. if I modify the ICD10 to make it inactive it then places this modification in the codes table. However, when searching for ALL items, it should exclude these from the search. Here’s the bug fix:
http://github.com/bradymiller/openemr/commit/1b72ddcea1dc4f0ee410d205316f0e0722198ba2
Kevin, this fix isn’t very pretty, but the problem here is that the $table_dot variable is needed for the search parameters, while the $table_dot_all (is ct.) is needed for the filter parameters. Let me know if you know of a better way to do this.
thanks,
-brady
OpenEMR
yehster wrote on Saturday, February 09, 2013:
What do you think about this?
https://github.com/yehster/openemr/commit/d50a23d5fc4a65130e7f9c84263e69b71b2cb38e
If a filter clause explicitly specifies a table, it doesn’t bother using $table_dot to append the clause.
It’s more generic.
A truly better way would be to define “-ALL-” as just another code type in the table and in the $code_external_tables array, but that’ll be a little trickier.
bradymiller wrote on Saturday, February 09, 2013:
Hi,
Your way is nice; more intuitive since the first thing I tried was to use the ct.ct_external, which broke because c. was pre-pended to it. Rather not make an -ALL- in the code type, because this option is likely going to die off at some point. I’ll bring in your solution and call it a day; I need to take a break from the code types module for a bit 
-brady
OpenEMR
bradymiller wrote on Saturday, February 09, 2013:
Hi,
Although I was trying to close the book on the code type module work I began thinking about the -ALL- setting a bit, and I realized that there was likely a way to include them all via mysql UNION calls. So, here’s the commit that does that. Notably, the UNION requires the same number of returned columns, so it pushed me to support a code_text_short description for all the code types in the metadata. Also further streamlined Administration->Services to support these features and added testing of it to the Testing scripts. Let me know your thoughts (if any):
http://github.com/bradymiller/openemr/commit/ac64f90701ba9d83d44973a083dac93ad6464348
-brady
OpenEMR
bradymiller wrote on Saturday, February 09, 2013:
Hi,
Extended on above commit to support any combination of code type searches in Administration-Services (multiple code types can be selected there). Here is the branch:
http://github.com/bradymiller/openemr/commits/all-search-code-type_2
-brady
OpenEMR
bradymiller wrote on Saturday, February 09, 2013:
Hi,
Ended up removing the -ALL- setting in code_set_search() and consolidating the internal codes algorithm with the external codes algorithm in code_set_search() (basically both are using the metadata now). Here’s the full commit which also includes theincorporation of short code descriptions and the searching off multiple code sets at a time in Administration->Services. This code is testing very well, so now placing it up for official code review:
http://github.com/bradymiller/openemr/commit/206212c78f908f7ad6b1a088abaa7227a2d6e552
-brady
OpenEMR