-
Notifications
You must be signed in to change notification settings - Fork 13
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
Joerd data #22
base: master
Are you sure you want to change the base?
Joerd data #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more general review:
Let's first focus on the providers. The sources
package makes sense. You can generalize this more even. Each provider should have the same basic structure, i.e. there should be a Base class which each provider inherits from, you can call that Provider.py
with a ProviderBase
class. You can make that an abstract class, see here if you want some inspiration. And read up a little about abstract classes and methods. It's object-oriented, which Python doesn't really do, so this is more of a hack, but it's an important principle. One abstract method would be download
which every subclass has to implement. Then you can define private methods for each provider class to make the download happen.
If you first do that, we can see how we implement the merging and the rest.
I think it's good that you treat SRTM and GMTED as one table in settings.yml
and just pull in GMTED when the area demands it. Maybe we can call the table terrestrial
instead of srtm
.
@@ -37,7 +37,8 @@ def create_app(script_info=None): | |||
log.info("Following provider parameters are active:\n" | |||
"Host:\t{host}\n" | |||
"DB:\t{db_name}\n" | |||
"Table:\t{table_name}\n" | |||
"Table1:\t{table_name_srtm}\n" | |||
"Table2:\t{table_name_composite}\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there is no composite table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isikl after struggling again to make this work in a fresh setup (my fault..), I couldn't get this to work. Mostly because the SETTINGS
are used wrongly in most places, e.g.
File "/deploy/app/openelevationservice/server/db_import/models.py", line 14, in <module>
if SETTINGS['provider_parameters']['tables']['terrestrial']:
KeyError: 'tables'
If you have a look where SETTINGS is used you'll see various places with non-valid keys according to the ops_settings_sample.yml
.
Can you please check and amend?
Should be fixed now. |
No description provided.