Conversation
Major rework, changes include: rewrite of method change of names to better reflect process creation of documentation additional testing example Works with both 2/3 python and backwards compatible.
|
I would leave the _build directory of the doc out of the repository, leave the client make the doc if needed. |
sposs
left a comment
sposs
left a comment
There was a problem hiding this comment.
A few comments and necessary fixes. Otherwise, great improvement!
geomag/latlon.py
Outdated
| if unit == 'degrees': | ||
| spherical_latitude = math.degrees(spherical_latitude) | ||
| spherical_longditude = math.degrees(spherical_longditude) | ||
| return (spherical_latitude, spherical_longditude, r) |
geomag/latlon.py
Outdated
| return "({} N,{} W)".format(*self._lat_lon()) | ||
|
|
||
| def _lat_lon(self): | ||
| return(self.lat, self.lon) |
geomag/latlon.py
Outdated
| The range is limited to within +-90 and +-180 degrees | ||
| ''' | ||
|
|
||
| if unit == 'degrees': |
There was a problem hiding this comment.
maybe use
if units in ["deg", "degrees"]:and
elif units in ["rad", "radians"]:below
doc/conf.py
Outdated
| # General information about the project. | ||
| project = 'GeoMag' | ||
| copyright = '2015, Todd Dembrey' | ||
| author = 'Todd Dembrey' |
doc/conf.py
Outdated
| # add these directories to sys.path here. If the directory is relative to the | ||
| # documentation root, use os.path.abspath to make it absolute, like shown here. | ||
| #sys.path.insert(0, os.path.abspath('.')) | ||
| sys.path.append(r'C:\Users\todd.dembrey\git\geomag') |
| time_adjusted_coefficients_cache = _gen_square_array(array_size, 0.0) | ||
|
|
||
| def __init__(self, file): | ||
| with open(file) as world_magnetic_model_file: |
There was a problem hiding this comment.
No check that the file exists... Will raise an error. Is this wanted?
There was a problem hiding this comment.
Yes, what else would you want it to do in this case?
There was a problem hiding this comment.
I usually anticipate the possible errors, but here, it's true that it will raise an exception... Good enough I guess.
geomag/world_magnetic_model.py
Outdated
| delta_time * self.coefficient_dot[n][m - 1]) | ||
|
|
||
|
|
||
| class WorldMagneticModel: |
geomag/world_magnetic_model.py
Outdated
| Example Usage: | ||
|
|
||
| >>> from geomag import WorldMagneticModel | ||
| >>> WorldMagneticModel().calc_mag_field(80,0).declination |
There was a problem hiding this comment.
Actually, it depends on the time... The value below should not remain in the doctring as doctest doesn't like it...
There was a problem hiding this comment.
Ah, you're referring to the output. I assumed it was the method call based on the lines shown above. I haven't run doctest on it, I guess removing it is the way to clear it up.
421564e
| ''' | ||
| self.data = MageneticModelData(world_magnetic_model_filename) | ||
| self.k = recursion_constants(self.data.array_size) | ||
|
|
There was a problem hiding this comment.
to be PEP8 compatible, all class members should be initialized in the constructor.
geomag/world_magnetic_model.py
Outdated
| return_list.append(variable_dict[variable]) | ||
| return return_list | ||
|
|
||
| if __name__ == '__main__': |
|
It's a big +1 for me. Great improvement!! |
|
Any chance of this getting merged at some point? PR is a fantastic rewrite - some colleagues lost a few hours working with master before realizing that Todd's fork existed and was more up to date. Otherwise, pointing people to todd-dembrey/geomag in a README might be helpful. |
|
@cmweiss I would also like to start working with the rewritten branch - are you still interested in maintaining this repository or would you like to hand it off (with credit of course) so development can continue? |
|
@cmweiss @todd-dembrey Any update on this? I'm using Django 2.0.4, which requires Python 3. |
|
I don't think there is any Python 2 specific code. It should work with Python 3. |
6 participants
Major rework, changes include:
Works with both 2/3 python and backwards compatible.
Happy to discuss further changes if required?