Skip to content

Conversation

Hagellach37
Copy link
Member

@Hagellach37 Hagellach37 commented Apr 28, 2020

work in progress

assure that:

further things addressed here:

@Hagellach37 Hagellach37 changed the title Completeness WIP: Completeness Apr 29, 2020
@Hagellach37
Copy link
Member Author

@Matthias-Schaub do you think that you have time during the next week to review to PR. It's getting bigger and bigger and I hope this is not too confusing, but I have the feeling that we are almost there.

So your comments will be valuable for us to get this merged into dev next week. :)

@TahiraU TahiraU changed the title WIP: Completeness Completeness May 11, 2020
@TahiraU TahiraU closed this May 11, 2020
@TahiraU TahiraU reopened this May 11, 2020
@TahiraU TahiraU self-requested a review May 11, 2020 15:04
1: BuildAreaProject,
2: FootprintProject,
3: ChangeDetectionProject,
3: BuildAreaProject,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BuildAreaProject is listed 3 times in the dict. Probably a mistyping or is it not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no this isn't a mistype, maybe it would be better if @Hagellach37 answers this modification. Basically because Completeness and ChangeDetection both have almost the same structure. Both projects require two urls.

super().__init__(project_draft)

# set group size
self.project_type = project_draft["projectType"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make the project type depended on the project draft? Project type of this class (BuildAreaProject) should always be 1.
It will work like this, but cleaner and less error prone would be to use a constant.

self.tileServer = self.get_tile_server(project_draft["tileServer"])

# get TileServerB for change detection and completeness type
if self.project_type in [3, 4]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the base class/ parent class BaseProject (base/base_project.py) not in the child class BuildAreaProject, because it apllies to a subset of child classes (with project type 3 or 4) not only to BuildAreaProject (type 1).

Or one would put this code into the associated child classes directly.

To be clear: Every project type should be represented by a single class. All project type classes are child classes of a base class where methods and variables are defined, which are shared/common across all project type classes.

)

apiKeyRequired = "AopsdXjtTu-IwNoCTiZBtgRJ1g7yPkzAi65nXplc-eLJwZHYlAIf2yuSY_Kjg3Wn"
apiKey = "AopsdXjtTu-IwNoCTiZBtgRJ1g7yPkzAi65nXplc-eLJwZHYlAIf2yuSY_Kjg3Wn"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API Keys should not be hardcoded. Since ths code is allready pushed to GitHub the API key is now public.
API Keys should be handled locally in evironment variales (Much like we allready do with the image api keys).

Do we need to change this API key because it is now public?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between apiKeyRequired vs. apiKey.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose there is no difference between apiKeyRequired and apiKey.


from mapswipe_workers.definitions import logger

url = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we define constants like url in definitions.py -> See for example IMAGE_URLS in definitions.py

Is this URL related? Could be that it does make sense to only define it here.

Copy link
Member

@TahiraU TahiraU May 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URL can be imported from definitions.py, that's correct. I will modify that!

BuildAreaProject.zoomLevel = int(18)
BuildAreaProject.tileServer = tile_server_dict

project_area = "/home/tahira/Public/LOKI/Attica/attica_maybe2.geojson"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path will only work on your computer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true! will change that

return True


a = tasks_to_geoJson(tasks, "attica_tasks_maybe2.geojson")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this script willl be executed directly it is a good practice to put the code which will be executed first (which are not defined in functions) inside if __name__ == "__main__": Statement.

See for further information: https://stackoverflow.com/questions/419163/what-does-if-name-main-do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok will change that!

@@ -0,0 +1,109 @@
from osgeo import ogr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the usecase for this module?

It is not been called from any other module. If it is only executed manually then it should be in mapswipe_workers/scripts/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ogr is used in tasks_to_geoJson() to create geojson file for the tasks

@Matthias-Schaub Matthias-Schaub requested a review from TahiraU May 24, 2020 09:35
@Hagellach37 Hagellach37 merged commit e466b87 into dev May 26, 2020
@Hagellach37 Hagellach37 mentioned this pull request Jun 2, 2020
Merged
@Hagellach37 Hagellach37 deleted the completeness branch March 3, 2021 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants