Skip to content

Mixed changes#53

Open
megies wants to merge 6 commits into
ozak:masterfrom
megies:mixed_changes
Open

Mixed changes#53
megies wants to merge 6 commits into
ozak:masterfrom
megies:mixed_changes

Conversation

@megies

@megies megies commented Nov 14, 2019

Copy link
Copy Markdown

WIP - DO NOT MERGE

only appropriate for NS-EW aligned raster but I guess thats true for the
whole code base

@ozak ozak left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What is the benefit of this? Just wondering? Less memory?

@ozak ozak left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not sure which duplication you refer to. The function is defined only once.

Comment thread georasters/georasters.py
Comment on lines 291 to 292
self.geot = geot
self.nodata_value = nodata_value

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Would there be a reason to not use the @property for the other properties?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There's no need, unless you want to add e.g. type checks when these get set.
Main reason to make those other attributes into properties is, they are derived values and should not be stored by itself but rather looked up from their "parents".

@ozak

ozak commented Nov 20, 2019

Copy link
Copy Markdown
Owner

Thanks for taking the time! If you are able to finish these let me know and I will merge the PR soon.

@megies

megies commented Nov 25, 2019

Copy link
Copy Markdown
Author

What is the benefit of this? Just wondering? Less memory?

Somehow this comment is not linked to code, so not sure what change you were commenting on..

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.

2 participants