Add activity schedule by location Mosaiq helper #1713
Conversation
|
We now have a means to have these helper functions be tested within. The PyMedPhys testing suite. See the following: Would you be able to include a test with this helper function? |
|
@SimonBiggs Sure thing, I'll give it a shot. |
|
@SimonBiggs I will need some guidance, if you please. It looks like the mimic Mosaiq database does not have a Schedule table, so I will need to add it with some dummy data for the test to work, right? How did you generate those CSV files with the dummy data for the tests? |
Of course. I believe I queried the relevant table filtered for a dummy physics patient to get a pandas data frame, and then saved the data frame to CSV using If you're able to give it a shot with that info, let me know. Otherwise I can try and see what I can dig up. Unfortunately, I no longer have access to a Mosaiq instance, so I can't trial and error myself... |
|
@SimonBiggs Thanks, that should help me get started. I'll report back in a couple of days once I've had time to test the waters. |
|
@SimonBiggs I've added the types for the Schedule table to the types_map.toml, but every time I save the modified file in VS Code, VS Code goes around sorting the entries alphabetically and moves them around. I have no idea what's triggering this behavior. Any way to stop this? |
When I edit and save that toml file in VSCode mine doesn't auto-sort. I suspect it may be an extension you have on your machine? When I run In saying that though, as long as the sorting is staying constant within the toml objects, the column order isn't important. The main downside of the sorting would be the changelog being polluted during PR review. |
|
It was worse than just sorting alphabetically within tables: it removed duplicate column names between tables. Thanks for the tip: I disabled the only TOML extension I had ( |
Ewww, gross :( |
|
Also, for that schedule table were you able to automatically extract that list from the SQL database? It might be worth adding a comment to the top of the file detailing the steps to extract new types. (Learning from my mistake last time, of not leaving enough breadcrumbs...) |
|
@SimonBiggs unfortunately, no, I just used my data dictionary. I don’t know SQL to do fancy queries like that! But I just did a quick search online and found how to do it. I will double-check what I wrote against that and leave a comment. I see your tables all end with RowVers in the TOML type map. That column isn’t in my data dictionary, but I added to my commit anyway. Not sure if it’s needed or not. |
|
'RowVers' is a column that was incorporated for use by an ORM adopted by MOSAIQ. |
|
Still working on this in my downtime. |
|
@SimonBiggs I wrote a little Also, a simple def mosaiq_table_to_toml_types_map(connection, table_name, path):
column_types = get_column_data_types(connection, table_name)
column_types_dict = {
table_name: pd.Series(column_types['data type'].values, index=column_types['column name']).to_dict()
}
with open(path, 'w') as f:
toml.dump(column_types_dict, f)Feedback? Where would you place this? |
|
I think helpers would be the perfect location. As a first pass, make it so
that the function itself doesn't edit the TOML file, instead just creates a
dictionary.
Then, create a small test that loads up the current toml file for one table
and compares the results of the function to that file. It'll be quite
circular, as the tests themselves will be loading up that toml file. But
nevertheless, it'll be quite helpful to validate the function and aid in
its future maintenance.
Thanks Marc! :)
…On Fri, 12 Aug 2022, 4:51 am Marc Chamberland, ***@***.***> wrote:
@SimonBiggs <https://github.com/SimonBiggs> I wrote a little
get_column_data_types() method and placed it in helpers.py... but I'm not
entirely sure if that's the best spot for it. Thoughts?
Also, a simple mosaiq_table_to_toml_types_map() could look something like:
def mosaiq_table_to_toml_types_map(connection, table_name, path):
column_types = get_column_data_types(connection, table_name)
column_types_dict = {
table_name: pd.Series(column_types['data type'].values, index=column_types['column name']).to_dict()
}
with open(path, 'w') as f:
toml.dump(column_types_dict, f)
Feedback? Where would you place this?
—
Reply to this email directly, view it on GitHub
<#1713 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSBK6ZSYXZWVENO7RG7GZ3VYVDTNANCNFSM55PYRM4A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Just an update to say this is still on my to do list. I've been busy with other stuff, but plan on finishing up those tests in the coming weeks. |
|
Hi @mchamberland, Just wanting to check how things were going on your end with this. 🙂 |
|
@SimonBiggs still planning on coming back to this! |
|
Awesome :) |
|
Hi @mchamberland, Just wanting to check in. Where do you think we might be up to on this PR? |
|
@SimonBiggs I am sitting down and looking at where I left off the tests as I write this. Thank you for your patience! 😅 |
Completely okay 🙂. Make sure the first thing you do is merge the current version of |
|
@SimonBiggs I wrote a test... but none of the test_mimicked_db.py tests pass. It seems like it's failing to setup the test database. Any suggestions? |
|
@SimonBiggs Oh, do I need to install MySQL or something similar? I assume that's what's going on... Anything I should know on how to configure it (I'm not even sure I'll be allowed to install it on my work laptop...) EDIT: Probably not it. Maybe I just need to try it on a computer where I have more rights... |
|
Well... if I push my changes, the tests run on GitHub, but you probably don't want to see how many times it will take me to sort things out! |
|
@SimonBiggs OK, some progress, but testing the column types against the types_map.toml fails because of the type casting workaround here. I guess my test function should handle those specific edge cases... |
|
I spent all morning unsuccessfully trying to get pymssql working on my M2 Mac. I give up. I will rely on pushing to my branch to have GitHub run the tests for me. We can just squash those commits together when the PR is ready. |
What version of python are you using? Is the problem connecting to your instance of SQL Server? |
Ah. I see: |
Things broke in the last few weeks... Previously, the solution was: And I think that's what was working for me at the time. |
|
@SimonBiggs the What gives? I had to put in an embarrassing workaround in my test. 🙈 |
|
Hi @mchamberland, I remember having quite a bit of pain here. But frustratingly I didn't leave enough breadcrumbs in the code detailing the choices. If mimics is changed to have it be varbinary does that cause issues with the tests? If not, then we should have it be like that. |
|
@SimonBiggs Good idea. Somehow, I thought there were other columns with type |
|
Thanks @mchamberland :) 🙏 |
|
@SimonBiggs well, I tried, and it broke a number of things in ways that were very obscure to me. I reverted back to my workaround. Will keep working on adding a few more tests for this new feature. Whew! |
|
Thanks 🙏 @mchamberland. Could you leave a few more comments scattered through the code that help people (future us?) get a feel for a bit of the weirdness on this one? |
No description provided.