Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Mapping streamline #12

Merged
merged 23 commits into from
Sep 29, 2016
Merged

Mapping streamline #12

merged 23 commits into from
Sep 29, 2016

Conversation

FrancescAlted
Copy link
Member

This is an attempt to simplify mapping.py as much as possible and promote it as a first-class module. Not ready for merging yet.

@FrancescAlted
mapping.py module made a first-class module (out of legacy)
30df128
@FrancescAlted
Remove map_dynaMap() as it is not reliable yet
02710e5
@FrancescAlted
Remove plot_sattracks() as it is currently broken and does not seems …
8a22f08 
…to be used
@FrancescAlted
Remove plot_imshow() as it is currently broken and does not seems to …
d45b91d 
…be used
@FrancescAlted
Remove get_OpenDAP() as it is currently broken and does not seems to …
d62a95c 
…be used
@FrancescAlted
More clean-up, refactoring and PEP8
6edf965
@FrancescAlted
Beautification of the XML snippets
be67476
@FrancescAlted
Start moving region definitions to a mapping_db.yml file. projection …
a3ebf0e 
…param is gone
@FrancescAlted
Added yaml to the list of requirements
4def1e0
@FrancescAlted
Making conda install the actual list of packages in requeriments.txt
daf4cb1
@FrancescAlted
Adding yaml package manually via pip, not conda
ee80a44
@FrancescAlted
Apparently the name for the package is pyyaml, not just yaml :P
8afa802
@coveralls
Copy link

coveralls commented Sep 16, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.5%) to 21.66% when pulling 8afa802 on mapping-streamline into 1c34011 on master.

@coveralls
Copy link

coveralls commented Sep 22, 2016 •
edited
Loading

Coverage Status

Coverage increased (+2.6%) to 23.732% when pulling 5ba18f5 on mapping-streamline into 1c34011 on master.

@coveralls
Copy link

coveralls commented Sep 23, 2016 •
edited
Loading

Coverage Status

Coverage increased (+3.4%) to 24.543% when pulling b16bffc on mapping-streamline into 1c34011 on master.

@coveralls
Copy link

coveralls commented Sep 23, 2016 •
edited
Loading

Coverage Status

Coverage increased (+3.4%) to 24.504% when pulling 41c18c0 on mapping-streamline into 1c34011 on master.

@FrancescAlted
Copy link
Member Author

This should be ready for merging. @jfburkhart would you like to review this one?

jfburkhart
jfburkhart approved these changes Sep 23, 2016
View reviewed changes
reflexible/mapping.py
"""
This is a core function, used throughout this module. It is called
by the :func:`get_FIGURE` function. If you want to create a new
region, this is where it should be added. Follow the protocol for the
Copy link
Member

Choose a reason for hiding this comment

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

Need to change the docstring to reflect the new environment variable

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 98da4cb

@coveralls
Copy link

coveralls commented Sep 28, 2016 •
edited
Loading

Coverage Status

Coverage increased (+3.4%) to 24.521% when pulling 98da4cb on mapping-streamline into 1c34011 on master.

@coveralls
Copy link

coveralls commented Sep 28, 2016 •
edited
Loading

Coverage Status

Coverage increased (+26.7%) to 47.878% when pulling 4082d5f on mapping-streamline into 1c34011 on master.

@FrancescAlted
Copy link
Member Author

@jfburkhart , in f3cde17 I have tried to clean-up the code of plotting.py a bit. In the process, I noticed that there are several places where we could be using uninitialized variables. I added comments with the "ERROR:" label on them. Could you have a look at them and tell me which could be proper defaults?

@coveralls
Copy link

coveralls commented Sep 28, 2016 •
edited
Loading

Coverage Status

Coverage increased (+26.8%) to 47.912% when pulling f3cde17 on mapping-streamline into 1c34011 on master.

jfburkhart
jfburkhart requested changes Sep 28, 2016
View reviewed changes
Copy link
Member

@jfburkhart jfburkhart left a comment

Choose a reason for hiding this comment

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

Regarding cax and cax2, in fact, I've had the error where these were uninitialized. They are the 'color axes' (or cax and cax2 for short). In some cases there are two cax ... in the case with the height plot overlaid, for example.

I agree we should find a better solution here.

@FrancescAlted
Copy link
Member Author

Added an issue for the initialized variables: #13

@FrancescAlted
Copy link
Member Author

I think this PR is already including more tasks than originally intended, so merging.

@FrancescAlted FrancescAlted merged commit c3961c3 into master Sep 29, 2016
@FrancescAlted FrancescAlted deleted the mapping-streamline branch September 29, 2016 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfburkhart jfburkhart jfburkhart requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FrancescAlted @coveralls @jfburkhart