Should data processing functions have their own modules?

GenericMappingTools / pygmt

A Python interface for the Generic Mapping Tools.

https://www.pygmt.org

BSD 3-Clause "New" or "Revised" License

758 stars 220 forks source link

Should data processing functions have their own modules? #807

Closed willschlitzer closed 3 years ago

willschlitzer commented 3 years ago

In #685, @weiji14 asked the question of if data processing modules like makecpt and grd2cpt should be split into their own Python scripts, much like the plotting modules will be. My opinion is they should be split up. The other files aren't nearly as big as base_plotting.py, but I think it's best to standardize all functions in this way, and it makes it easier to find the function you're looking for (I wasn't sure if grd2cpt should be under mathops.py or grdops.py?).

I'm happy to do it. I'm assuming the process would be similar to the changes in base_plotting.py, and just involve copying the functions to their own scripts in pygmt.src, but how do we "tell" PyGMT where to look for it beyond the init file, as there isn't a base_plotting-equivalent script to import into?

michaelgrund commented 3 years ago

I completely agree with you @willschlitzer. Splitting up all the modules allows to arrange everything in a consistent way. Would also support you in this intention if you need help.

seisman commented 3 years ago

The other files aren't nearly as big as base_plotting.py, but I think it's best to standardize all functions in this way, and it makes it easier to find the function you're looking for

I agree with you.

how do we "tell" PyGMT where to look for it beyond the init file, as there isn't a base_plotting-equivalent script to import into

I think we need to change these lines in __init__.py:

https://github.com/GenericMappingTools/pygmt/blob/23454867d4785112281b397a08d7dac109d28f74/pygmt/__init__.py#L14-L24

willschlitzer commented 3 years ago

@seisman So would we completely eliminate the files like mathops.py and gridops.py and just import directly into pygmt/__init__.py with something like from pygmt.src.makecpt import makecpt? If so, how will that affect Sphinx building the documentation?

seisman commented 3 years ago

So would we completely eliminate the files like mathops.py and gridops.py and just import directly into pygmt/__init__.py with something like from pygmt.src.makecpt import makecpt?

Yes to me.

If so, how will that affect Sphinx building the documentation?

I'm not sure. We need to give it a try.

weiji14 commented 3 years ago

Do we want to get this in for PyGMT v0.3.0 or the next release (v0.3.1 or v0.4.0)?

seisman commented 3 years ago

I'm OK to both. I think I can finish PR #832 soon.

weiji14 commented 3 years ago

Ok, but we should merge #808 before #832. One step at a time ;)

seisman commented 3 years ago

These two PRs won't cause conflicts.

weiji14 commented 3 years ago

There might be a minor conflict in the pygmt/src/__init__.py file, but yeah, shouldn't be too hard to resolve hopefully.

seisman commented 3 years ago

Todo list after PR #832:

[x] config: wrapper of GMT's set command. pygmt/config.py? pygmt/src/config.py? pygmt/src/set.py?
[x] GMTDataArrayAccessor: pygmt/datatypes.py?
[x] x2sys_init and x2sys_cross: do we want to put these functions into two files or one file pygmt/src/x2sys.py (#951)

seisman commented 3 years ago

Ping @weiji14 to separate x2sys_init and x2sys_cross.

weiji14 commented 3 years ago

Todo list after PR #832:

* [ ]  `config`: wrapper of GMT's `set` command. `pygmt/config.py`? `pygmt/src/config.py`? `pygmt/src/set.py`?
* [ ]  `GMTDataArrayAccessor`: `pygmt/datatypes.py`?

Right, do we want to move the config and GMTDataArrayAccessor classes out of pygmt/modules.py? I wouldn't consider them 'data processing' modules, they're more about plot configuration and metadata related.

seisman commented 3 years ago

We should. I think pygmt.config should be in pygmt/src/config.py, but I'm not sure where to put GMTDataArrayAccessor.

maxrjones commented 3 years ago

We should. I think pygmt.config should be in pygmt/src/config.py, but I'm not sure where to put GMTDataArrayAccessor.

Could something like either pygmt/src/accessors.py or pygmt/src/xarray_extension.py work?

weiji14 commented 3 years ago

We should. I think pygmt.config should be in pygmt/src/config.py, but I'm not sure where to put GMTDataArrayAccessor.

Could something like either pygmt/src/accessors.py or pygmt/src/xarray_extension.py work?

I'm ok with moving pygmt.config to pygmt/src/config.py since gmt set is a pure GMT function. But I'm inclined to keep GMTDataArrayAccessor outside of the src folder. Maybe do the following:

Move class config from pygmt/modules.py to pygmt/src/config.py
Rename pygmt/modules.py to pygmt/accessors.py

seisman commented 3 years ago

Move class config from pygmt/modules.py to pygmt/src/config.py

Rename pygmt/modules.py to pygmt/accessors.py

Sounds good.

maxrjones commented 3 years ago

Move class config from pygmt/modules.py to pygmt/src/config.py

Rename pygmt/modules.py to pygmt/accessors.py

Sounds good.

I can help with this if needed.