add key to get confirmed trip in read_data #798
Conversation
| time_query=None, geo_query=None) | ||
| def read_data(uuid=None,key='cleaned_trip'): | ||
| trips = None | ||
| if key == 'confirmed_trip': |
There was a problem hiding this comment.
This is functionally correct, but poor design. It would require modification every time we needed to switch to a new type of trip.
Please pass the key type in directly. The calling site will then need to pass in esda.CONFIRMED_TRIP_KEY or esda.CLEANED_TRIP_KEY.
There was a problem hiding this comment.
But all existing files that run this pipeline doesn't pass in any key type. Naomi's files don't pass in keys and they use CLEANED_TRIP_KEY. I am afraid your suggested change would need to change many files...
Also, I don't see the difference after make such change. If you need to pass in esda.CONFIRMED_TRIP_KEY or esda.CLEANED_TRIP_KEY anyway, it would still choose one type of trip(the line of codetrips = esda.get_entries(esda.CONFIRMED_TRIP_KEY, uuid, time_query=None, geo_query=None) only choose one trip)
. And all existing files have to modified to pass in the key.
Can I not make this change? I think that's good to keep the existing files work and also flexible for future files.
If in the future, we need to get another type of trip, we indeed to modify this file, just add if statement and get_entries. But that doesn't hurt all existing files.
There was a problem hiding this comment.
you would have to change all the files even with your change, though. Your check is if 'confirmed_trip':... elif 'cleaned_trip'
If it is neither, what will your code do?
There was a problem hiding this comment.
Then we will see what kind of trip other people need to get. It indeed needs to add other if statements and other entry keys but we don't need to modify other files.
Even if I change this file now, that one line of codetrips = esda.get_entries(esda.CONFIRMED_TRIP_KEY, uuid, time_query=None, geo_query=None) just read one kind of trips. Then I need to add all keys and maybe some of them wouldn't be used. I think we just make changes as needed.
There was a problem hiding this comment.
I don't need to change all files. Before I made this change, Naomi just read in cleaned trips, so I set cleaned_trip to default, other existing files can run correctly without passing in key type. the evaluation I am doing (needs confirmed trip, different from the existing files) needs to pass inconfirmed_trip and the pipeline can choose confirmed trip key.
There was a problem hiding this comment.
you're right! I missed that detail.
But you can do that with esda.CLEANED_TRIP as well. Just set that to the default.
Fundamentally, I don't see what creating a new string and adding if checks gives you instead of just using esda.* directly.
| def read_data(uuid=None,key='cleaned_trip'): | ||
| trips = None | ||
| if key == 'confirmed_trip': | ||
| trips = esda.get_entries(esda.CONFIRMED_TRIP_KEY, uuid, | ||
| time_query=None, geo_query=None) | ||
| elif key == 'cleaned_trip': | ||
| trips = esda.get_entries(esda.CLEANED_TRIP_KEY, uuid, | ||
| time_query=None, geo_query=None) |
There was a problem hiding this comment.
| def read_data(uuid=None,key='cleaned_trip'): | |
| trips = None | |
| if key == 'confirmed_trip': | |
| trips = esda.get_entries(esda.CONFIRMED_TRIP_KEY, uuid, | |
| time_query=None, geo_query=None) | |
| elif key == 'cleaned_trip': | |
| trips = esda.get_entries(esda.CLEANED_TRIP_KEY, uuid, | |
| time_query=None, geo_query=None) | |
| def read_data(uuid=None,key=esda.CLEANED_TRIP): | |
| trips = esda.get_entries(key, uuid, | |
| time_query=None, geo_query=None) |
|
@corinne-hcr Can you please indicate the testing done before I merge? |
|
OK. All checks passed. |
|
I don't think the checks actually cover this code path (which is a different fix). But good to know that it works in the notebook as well. |
* add key to get confirmed trip in read_data * modify read_data function
No description provided.