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.

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

Mapbase converts crln_obs to hgln_obs if it is found in the header #2946

Merged
merged 13 commits into from Mar 13, 2019

Conversation

Projects
None yet
5 participants
@dodoextinct
Copy link
Contributor

commented Feb 28, 2019

Description

Fixes #2942

@pep8speaks

This comment has been minimized.

Copy link

commented Feb 28, 2019

Hello @dodoextinct! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 30:1: E302 expected 2 blank lines, found 1
Line 200:1: W293 blank line contains whitespace
Line 201:1: E302 expected 2 blank lines, found 1
Line 202:101: E501 line too long (133 > 100 characters)

Comment last updated at 2019-03-13 13:49:19 UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Feb 28, 2019

Thanks for the pull request @dodoextinct! Everything looks great!

@nabobalis nabobalis added this to the 1.0 milestone Feb 28, 2019

@Cadair
Copy link
Member

left a comment

Thanks for the PR!

I am not sure why all the tests have failed, can you investigate?

Also, could you write a test in the test_genericmap file, that uses the test HMI file we have (which uses crln/t_obs).

Show resolved Hide resolved sunpy/map/mapbase.py Outdated
@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 28, 2019

Thanks for the PR!

I am not sure why all the tests have failed, can you investigate?

Also, could you write a test in the test_genericmap file, that uses the test HMI file we have (which uses crln/t_obs).

The error is when the code is performing subtraction,

Can only apply 'subtract' function to dimensionless quantities when other argument is not a quantity (unless the latter is all zero/infinity/nan)

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 28, 2019

Thanks for the PR!

I am not sure why all the tests have failed, can you investigate?

Also, could you write a test in the test_genericmap file, that uses the test HMI file we have (which uses crln/t_obs).

I think the error is when performing subtraction it is saying,

"The value must be a valid Python or Numpy numeric type."

Can you help?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Feb 28, 2019

The error seems to imply that one variable is a astropy.unit and the other is not. Therefore, it can not be subtracted. I would see what type those two variables are and go from there.

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 28, 2019

The error seems to imply that one variable is a astropy.unit and the other is not. Therefore, it can not be subtracted. I would see what type those two variables are and go from there.

What can I do to help?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Feb 28, 2019

I would see what type those two variables within the subtraction are and go from there.

Show resolved Hide resolved sunpy/map/mapbase.py Outdated
@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 1, 2019

The error seems to imply that one variable is a astropy.unit and the other is not. Therefore, it can not be subtracted. I would see what type those two variables are and go from there.

@dodoextinct dodoextinct closed this Mar 1, 2019

@dodoextinct dodoextinct deleted the dodoextinct:patch-1 branch Mar 1, 2019

@dodoextinct dodoextinct restored the dodoextinct:patch-1 branch Mar 1, 2019

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 1, 2019

I would see what type those two variables within the subtraction are and go from there.

Did you see the type?

@dodoextinct dodoextinct reopened this Mar 1, 2019

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 1, 2019

Sorry, I do not follow.

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 2, 2019

Sorry, I do not follow.

You were going to inspecting the types of the variables like you said?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 2, 2019

Ah sorry, I meant that you should have a look to see what's wrong.

The numpy issue probably means it needs to be updated (if possible) or downgraded. Hopefully they could fix it.

@Cadair

This comment was marked as outdated.

Copy link
Member

commented Mar 2, 2019

@dodoextinct I think a quick poke around in this with the debugger should be sufficient to get this working. If you run pytest sunpy/coordinates --pdb you will be presented with a python debugger where the error occurs and you can inspect the variables and see what's going on.

@Akram9

This comment was marked as outdated.

Copy link
Contributor

commented Mar 3, 2019

Doesn't get_sun_L0(self.date) give astropy.coordinates.angles.Longitude type?
That doesn't seem to convert to float on applying float().

Also, numpy issue can be solved by upgrading to v1.16 using pip.

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 4, 2019

@dodoextinct I think a quick poke around in this with the debugger should be sufficient to get this working. If you run pytest sunpy/coordinates --pdb you will be presented with a python debugger where the error occurs and you can inspect the variables and see what's going on.

Okay, I can try that.

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 4, 2019

Doesn't get_sun_L0(self.date) give astropy.coordinates.angles.Longitude type?
That doesn't seem to convert to float on applying float().

Also, numpy issue can be solved by upgrading to v1.16 using pip.

Exactly it doesn't get converted into float.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 4, 2019

Doesn't get_sun_L0(self.date) give astropy.coordinates.angles.Longitude type?
That doesn't seem to convert to float on applying float().
Also, numpy issue can be solved by upgrading to v1.16 using pip.

Exactly it doesn't get converted into float.

Yes, in this case you want to keep them as astropy.units and fix the subtract another way.
You can see the documentation here: http://docs.astropy.org/en/stable/units/

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 8, 2019

Doesn't get_sun_L0(self.date) give astropy.coordinates.angles.Longitude type?
That doesn't seem to convert to float on applying float().
Also, numpy issue can be solved by upgrading to v1.16 using pip.

Exactly it doesn't get converted into float.

Yes, in this case you want to keep them as astropy.units and fix the subtract another way.
You can see the documentation here: http://docs.astropy.org/en/stable/units/

I have tried changing units and debugging it and I am afraid, I couldn't solve the problem. Can anybody help?

@nabobalis nabobalis force-pushed the dodoextinct:patch-1 branch from 6b2cd00 to 57e5ee0 Mar 8, 2019

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 8, 2019

Hey @dodoextinct, I had a look at the code and discovered the issue was something else. It seemed the indentation was the issue that broke the code.

Regarding what is left to do on this PR, we need a changelog entry and a unittest for the new behaviour.

I also rebased the PR since we have had quite some changes to master.
So locally you will need to do:

git remote update
git checkout master
git branch -D patch-1
git checkout -b origin/patch-1

That should restore your local version to what is here.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 9, 2019

Looks good to go but just a unit test left now.

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 10, 2019

Looks good to go but just a unit test left now.

Do I have to make changes in this code for the test?

def test_heliographic_longitude(generic_map):
assert generic_map.heliographic_longitude == 0.
https://github.com/sunpy/sunpy/blob/76c87eb450d3b2c1f8ec43d394d33ab507d42cf5/sunpy/map/tests/test_mapbase.py#L182

@dodoextinct

This comment was marked as outdated.

Copy link
Contributor Author

commented Mar 10, 2019

Looks good to go but just a unit test left now.

Do I have to make changes in the following code?

def test_heliographic_longitude(generic_map): assert generic_map.heliographic_longitude == 0.
I am a bit confused. Can you help?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 10, 2019

You will want to edit that file, but I don't think you want to edit a test, but create a new one.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 10, 2019

You can see cadair's comment at the top for what file to test as well.

dodoextinct added some commits Mar 10, 2019

@dodoextinct

This comment was marked as resolved.

Copy link
Contributor Author

commented Mar 10, 2019

You can see cadair's comment at the top for what file to test as well.

I have written the test, but I am afraid, it must be wrong. I am terribly confused about it. Can you guide me?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Mar 10, 2019

You can see cadair's comment at the top for what file to test as well.

I have written the test, but I am afraid, it must be wrong. I am terribly confused about it. Can you guide me?

I would suggest having a look at the other tests (test_carrington_longitude and test_heliographic_latitude in particular) in the file to see how the other tests are written.

But you essentially need to create a test that loads in the https://github.com/sunpy/sunpy/blob/master/sunpy/data/test/resampled_hmi.fits
and passes it into the generic map function and checks that the correct change was made to the attribute, that your code changed.

You should read up on pytest as well to try and understand the framework. Also I found this introduction.

When you write a test you will also want to run it locally to make sure it passes.
pytest <path to file you want to run> should work.

dodoextinct added some commits Mar 11, 2019

@nabobalis nabobalis modified the milestones: 1.0, 0.9.7 Mar 11, 2019

@Cadair
Copy link
Member

left a comment

This is looking great @dodoextinct.

I think just the suggestions made on this and it's good to go.

Show resolved Hide resolved sunpy/map/tests/test_mapbase.py Outdated
Show resolved Hide resolved sunpy/map/tests/test_mapbase.py Outdated

Cadair and others added some commits Mar 13, 2019

Update sunpy/map/tests/test_mapbase.py
Co-Authored-By: dodoextinct <yashkmkrishan@gmail.com>
Update sunpy/map/tests/test_mapbase.py
Co-Authored-By: dodoextinct <yashkmkrishan@gmail.com>
Update changelog/2946.bugfix.rst
Co-Authored-By: dodoextinct <yashkmkrishan@gmail.com>
@Cadair

Cadair approved these changes Mar 13, 2019

@Cadair Cadair merged commit 572304b into sunpy:master Mar 13, 2019

9 checks passed

ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip-install Your tests passed on CircleCI!
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190313.12 succeeded
Details
@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Thanks @dodoextinct

@dodoextinct dodoextinct deleted the dodoextinct:patch-1 branch Mar 13, 2019

nabobalis added a commit to nabobalis/sunpy that referenced this pull request Apr 18, 2019

Mapbase converts crln_obs to hgln_obs if it is found in the header (s…
…unpy#2946)

* Solving Issue sunpy#2942

* update fix for crln_obs in mapbase

* Mapbase converts crln_obs to hgln_obs

* Writing test for test_hmi_file

* test for hmi file

* test for hmi_file

* Creating test for HMI_file

* Updated the test for HMI_file.

* Update sunpy/map/tests/test_mapbase.py

Co-Authored-By: dodoextinct <yashkmkrishan@gmail.com>

* Update sunpy/map/tests/test_mapbase.py

Co-Authored-By: dodoextinct <yashkmkrishan@gmail.com>

* Update changelog/2946.bugfix.rst

Co-Authored-By: dodoextinct <yashkmkrishan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.