Skip to content

Integrate Placemark and TileSource?#95

Open
ghost wants to merge 7 commits into
mainfrom
donuty-party
Open

Integrate Placemark and TileSource?#95
ghost wants to merge 7 commits into
mainfrom
donuty-party

Conversation

@ghost

@ghost ghost commented Mar 14, 2021

Copy link
Copy Markdown

This would make core.py dependent on placemark.py, tilesource.py, and all of their config files. It would rip out the guts of getTile() and getTile_3x3(); these would wrap TileSource.tile() and TileSource.tile_3x3().

I'm using relative imports, so you can no longer just run the script with python core.py; you need to use python -m Library.core. If there's a way to avoid this, let me know.

This is major stuff, but the advantages, in my opinion, are:

  • Moving placemarks and tile sources to individual config files lets us and new programmers make changes without breaking code.
  • Making the code modular and object-oriented makes our products easier for new programmers to interface with and puts the complexity of the underlying implementations squarely on us.
  • These implementations have plenty of comments to clarify what's going on for anyone who wants to take a peek under the hood.
  • The combo of placemarks and tile sources resolves issue Implement "Points of Interest" table #90

donuty-party added 2 commits March 13, 2021 21:34
This makes core.py dependent on relative imports to Placemark and TileSource.

core.py loads configs for these classes when it is run.

Original functions are eviscerated.
@ghost ghost requested review from bojiang423, kkmcgg and ynandal99 March 14, 2021 01:50
donuty-party added 2 commits March 13, 2021 22:27
This might not be helpful if the files fail to import for any other reason than core.py being run directly.
@kkmcgg

kkmcgg commented Mar 14, 2021

Copy link
Copy Markdown
Contributor

Hey @donuty-party Good effort here.

My initial thoughts are we may have an easier time handling any of our internal file structure through the python library pandas. Pandas may be good for us in general as well as it can help with data handling in a number of ways - data cleaning for example, summarizing etc.. I think in general the syntax of pandas would be good for us all to get some experience with as it comes up a fair bit in the bigger python data science community.

Perhaps we can add this as a topic for our Tuesday meeting so we can all have a change to discuss and learn from what you've done and what functionality you're proposing. People may benefit from seeing the pull request procedure general as well.

I have been foreseeing the need for a Tile object, wherein we can carry some information along with the array such as data source, extents, resolution, and some other meta data. You've pushed this along well.

Reformat to make flake8 and pylint happier.
@ghost

ghost commented Mar 16, 2021

Copy link
Copy Markdown
Author

@kevinkmcguigan, when you say our internal file structure should be handled through pandas, are you thinking that the directory-files structure should be replaced with a single file for each of Placemarks and TileSources, in something like CSV, to be ingested into a pandas data frame?

@ghost

ghost commented Mar 17, 2021

Copy link
Copy Markdown
Author

Even if this is the wrong direction for the project, writing something functional is helping me get more comfortable with Python, and with GitHub. At work, I didn't have many opportunities to use Python, and I assume I wouldn't have been allowed to use GitHub – I didn't use any revision control at all – so this is good.

@kkmcgg

kkmcgg commented Mar 17, 2021

Copy link
Copy Markdown
Contributor

Hey Jeff! I'm happy you're putting a fair bit of effort into this - I agree its a good place to learn things like this! You're pushing forward into some territory I don't typically take advantage of in python and git! I don't tend to use TOML config files for example. That's cool! Sure beats XML it seems! Learning and experience is good all around.

I know you've been putting a fair effort into this and its in a holding pattern. I may jump ahead of our Tuesday discussion on it merge in your changes shortly so that we can continue on with some of the image processing and machine learning parts. I am worried about some of the added complexity especially when it comes to saving and loading various models but we can debug that as we go perhaps.

@ghost

ghost commented Mar 17, 2021

Copy link
Copy Markdown
Author

I am worried about some of the added complexity especially when it comes to saving and loading various models but we can debug that as we go perhaps.

@kevinkmcguigan, I'm less sure myself about the model loading feature, since it makes assumptions I'm not sure will remain valid if we moved to Keras or something. The code itself isn't as good as the Placemark and TileSource code either. I'm going to take that out and put it into another branch.

donuty-party added 2 commits March 17, 2021 12:14
Relative imports force us to change how we load the core module — `python -m Library.core` instead of `python Library/core.py`. Absolute imports work fine and don't force this change.
Revert loadModel() and saveModel() to their originals and remove dependency on model.py.
@kkmcgg

kkmcgg commented Mar 23, 2021

Copy link
Copy Markdown
Contributor

We want the placemark and tile source functionality included - we will abandon the model handling script for now.

@kkmcgg kkmcgg added this to the Deploy Python Package milestone Mar 23, 2021
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.

1 participant