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

PR for issues #12, #25, #47, #28 #45

Merged
merged 23 commits into from
Feb 22, 2021
Merged

PR for issues #12, #25, #47, #28 #45

merged 23 commits into from
Feb 22, 2021

Conversation

gregstarr
Copy link
Contributor

remove documentation for the non-return_all case of basevectors_apex

this PR is to resolve #12

@gregstarr
Update apex.py
8e9c308 
remove documentation for the non-`return_all` case of `basevectors_apex`
@gregstarr
Copy link
Contributor Author

gregstarr commented Feb 12, 2021 •
edited
Loading

The rest of the docstring was removed here: 7eeba12

@gregstarr
Copy link
Contributor Author

as for the tutorial mentioned in #12 I don't think I am qualified to create that

@gregstarr
Update apex.py
b0d20ca 
adding a note to the `Apex` class to address aburrell#25
@gregstarr
Copy link
Contributor Author

also added a note in Apex to address #25

@gregstarr gregstarr changed the title Update apex.py docs to address #12 Update apex.py docs to address #12 and #25 Feb 12, 2021
@coveralls
Copy link

coveralls commented Feb 12, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 588

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%
Totals Coverage Status
Change from base Build 555: 0.0%
Covered Lines: 0
Relevant Lines: 0
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 557

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%
Totals Coverage Status
Change from base Build 555: 0.0%
Covered Lines: 0
Relevant Lines: 0
💛 - Coveralls

@gregstarr
Copy link
Contributor Author

#47 was caused by fa.cofrm supplying a relative path to igrf13coeffs.txt which wouldn't resolve properly unless the user's base path was src/fortranapex. I did not know how to solve this with fortran, but python makes it very easy to get file path / directory names so I just use the path of igrf13coeffs.txt relative to apex.py and pass it into fa.cofrm. I regenerated fortranapex.pyf using

python -m numpy.f2py .\apex.f .\apexsh.f90 .\checkapexsh.f90 .\magfld.f .\makeapexsh.f90 -m fortranapex -h fortranapex.pyf

Note that I did not include igrf.f90 because I don't think ALLOCATABLE inputs are compatible with f2py and no functions in igrf.f90 are used directly in apexpy (https://stackoverflow.com/questions/34579769/f2py-error-with-allocatable-arrays/34708146 / https://scicomp.stackexchange.com/questions/6961/f2py-with-allocatable-and-assumed-shape-arrays).

@gregstarr
Copy link
Contributor Author

Also had to remove the print statements from igrf.f90 so that the command line tests would pass and just so that the command line output remains the same.

@gregstarr gregstarr changed the title Update apex.py docs to address #12 and #25 PR for issues #12, #25, #47, #28 Feb 15, 2021
@gregstarr
Copy link
Contributor Author

adding proposed fix for #28

aburrell
aburrell reviewed Feb 16, 2021
View reviewed changes
Copy link
Owner

@aburrell aburrell left a comment

Choose a reason for hiding this comment

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

Found a few things, but haven't had a chance to run it locally yet.

src/apexpy/apex.py Outdated Show resolved Hide resolved
src/apexpy/apex.py Outdated Show resolved Hide resolved
src/fortranapex/magfld.f Show resolved Hide resolved
gregstarr and others added 7 commits February 16, 2021 11:21
@gregstarr @aburrell
Update src/apexpy/apex.py
f68dc7c 
Co-authored-by: Angeline Burrell <aburrell@users.noreply.github.com>
@gregstarr @aburrell
Update src/apexpy/apex.py
0914e5b 
Co-authored-by: Angeline Burrell <aburrell@users.noreply.github.com>
@aburrell
TST: removed strict flag
31d8dd0 
Removed deprecated `strict` flag.
@aburrell
STY: updated formatting
37b58f4 
Updated formatting for section headers.
@aburrell
TST: updated filename construction
92efa16 
Updated filename construction, using `os.path.join` in all instances.
@aburrell
DOC: clarified parameters
aa90952 
Clarified the parameters in the docstring.
@aburrell
DOC: added a note about the geodetic system
d3002f3 
Added a note about the geodetic system to the README.
@aburrell aburrell linked an issue Feb 19, 2021 that may be closed by this pull request
What geodetic system is used? #25
Closed
@aburrell
TST: added unit test for equator conversions
8c5de57 
Added a unit test based on the edge case provided by @bharding512
@aburrell
Copy link
Owner

Ok, everything passes locally for me. I reviewed all the linked issues and this addresses everything except the tutorial. I added a note to that issue and removed it from the current milestone. Will provide approval Monday, once the unit tests are done.

aburrell and others added 4 commits February 19, 2021 17:06
@aburrell
STY: removed trailing whitespace
7ad0842 
Removed trailing whitespace for PEP8 compliance.
@aburrell
STY: removed more trailing whitespace
4192f3b 
Removed yet more trailing whitespace.
@gregstarr
Update test_cmd.py
5829657 
adding whitespace for pep8 compliance
@gregstarr
Update test_Apex.py
12fcc7c 
removing trailing whitespace to satisfy codacy
@aburrell aburrell merged commit fbe4e09 into aburrell:develop Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aburrell aburrell aburrell approved these changes

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

Successfully merging this pull request may close these issues.

What geodetic system is used?
3 participants
@gregstarr @coveralls @aburrell