oturns / geosnap

The Geospatial Neighborhood Analysis Package
https://oturns.github.io/geosnap-guide
BSD 3-Clause "New" or "Revised" License
244 stars 32 forks source link

Addition of Geosilhouettes #256

Closed AnGWar26 closed 4 years ago

AnGWar26 commented 4 years ago

Thought it would be best if I gave you the opportunity to comment on this as I work @knaaptime.

Todo:

Updated 8/19

codecov[bot] commented 4 years ago

Codecov Report

Merging #256 into master will decrease coverage by 24.54%. The diff coverage is 13.54%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #256       +/-   ##
===========================================
- Coverage   79.24%   54.70%   -24.55%     
===========================================
  Files          12       12               
  Lines         853      998      +145     
===========================================
- Hits          676      546      -130     
- Misses        177      452      +275     
Impacted Files Coverage Δ
geosnap/_community.py 39.28% <5.69%> (-26.18%) :arrow_down:
geosnap/analyze/analytics.py 65.75% <43.75%> (-6.43%) :arrow_down:
geosnap/analyze/dynamics.py 22.58% <0.00%> (-77.42%) :arrow_down:
geosnap/io/storage.py 24.66% <0.00%> (-70.67%) :arrow_down:
geosnap/io/util.py 48.83% <0.00%> (-30.24%) :arrow_down:
geosnap/_data.py 74.11% <0.00%> (+3.52%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b1903b6...641fdb6. Read the comment docs.

knaaptime commented 4 years ago

cool, lets talk about this a bit during the meeting on tuesday. It'll be a little easier to describe what i mean about stashing the index so we dont have to fillna

Also i think it would be better to switch the ordering of the plotting nomenclature, e.g. instead of ***_plot(...), i think it would be better to do plot_***(...)

knaaptime commented 4 years ago

Handle input when an aspatial model is passed to a spatial diagnostic Currently throws what might be a cryptic error to most users 'NoneType' object has no attribute 'sparse', referring to the absence of a W object

one thing we could do is add a little metadata attribute to the ModelResults class that stores whether the model is spatial or aspatial. That way we can add a simple check if users pass the wrong type of model to the wrong diagnostic

knaaptime commented 4 years ago

awesome! looks like this has everything we talked about yesterday. Everything working as expected?

AnGWar26 commented 4 years ago

As far as I can tell everything works as expected, and at this point I would designate this ready for review. However, this is such a big PR (+500 lines), and with your understanding of these statistics being better than mine, I would encourage you to test it out yourself before merging.

Let me know if these needs anymore changes.