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

Should data processing functions have their own modules? #807

Closed
willschlitzer opened this issue Jan 27, 2021 · 17 comments · Fixed by #1087
Closed

Should data processing functions have their own modules? #807

willschlitzer opened this issue Jan 27, 2021 · 17 comments · Fixed by #1087
Labels
maintenance Boring but important stuff for the core devs
Milestone

Comments

@willschlitzer
Copy link
Contributor

willschlitzer commented Jan 27, 2021

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?

@willschlitzer willschlitzer added the question Further information is requested label Jan 27, 2021
@michaelgrund
Copy link
Member

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
Copy link
Member

seisman commented Jan 27, 2021

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:

pygmt/pygmt/__init__.py

Lines 14 to 24 in 2345486

# Import modules to make the high-level GMT Python API
from pygmt import datasets
from pygmt.figure import Figure
from pygmt.filtering import blockmedian
from pygmt.gridding import surface
from pygmt.gridops import grdcut, grdfilter
from pygmt.mathops import makecpt
from pygmt.modules import GMTDataArrayAccessor, config, grdinfo, info, which
from pygmt.sampling import grdtrack
from pygmt.session_management import begin as _begin
from pygmt.session_management import end as _end

@willschlitzer
Copy link
Contributor Author

willschlitzer commented Jan 28, 2021

@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
Copy link
Member

seisman commented Jan 29, 2021

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
Copy link
Member

weiji14 commented Feb 4, 2021

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

@seisman
Copy link
Member

seisman commented Feb 4, 2021

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

@weiji14
Copy link
Member

weiji14 commented Feb 4, 2021

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

@seisman
Copy link
Member

seisman commented Feb 4, 2021

These two PRs won't cause conflicts.

@weiji14
Copy link
Member

weiji14 commented Feb 5, 2021

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

@seisman
Copy link
Member

seisman commented Feb 7, 2021

Todo list after PR #832:

@seisman
Copy link
Member

seisman commented Feb 22, 2021

Ping @weiji14 to separate x2sys_init and x2sys_cross.

weiji14 added a commit that referenced this issue Feb 22, 2021
Move the x2sys modules originally wrapped in #546.
The single x2sys.py has been separated into x2sys_init.py
and x2sys_cross.py inside the src/ folder as per #807.
weiji14 added a commit that referenced this issue Feb 22, 2021
Move the x2sys modules originally wrapped in #546.
The single x2sys.py has been separated into x2sys_init.py
and x2sys_cross.py inside the src/ folder as per #807.
weiji14 added a commit that referenced this issue Feb 22, 2021
…oss (#951)

Move the x2sys modules originally wrapped in #546.
The single x2sys.py has been separated into x2sys_init.py
and x2sys_cross.py inside the src/ folder as per #807.
@weiji14
Copy link
Member

weiji14 commented Feb 22, 2021

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
Copy link
Member

seisman commented Feb 22, 2021

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

@maxrjones
Copy link
Member

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
Copy link
Member

weiji14 commented Mar 13, 2021

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:

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

@seisman
Copy link
Member

seisman commented Mar 13, 2021

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

Sounds good.

@maxrjones
Copy link
Member

  • 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.

@seisman seisman added this to the 0.4.0 milestone Mar 15, 2021
@maxrjones maxrjones added enhancement Improving an existing feature and removed enhancement Improving an existing feature labels Mar 20, 2021
@seisman seisman added maintenance Boring but important stuff for the core devs and removed question Further information is requested labels Mar 20, 2021
@maxrjones maxrjones mentioned this issue Mar 21, 2021
5 tasks
sixy6e pushed a commit to sixy6e/pygmt that referenced this issue Dec 21, 2022
…oss (GenericMappingTools#951)

Move the x2sys modules originally wrapped in GenericMappingTools#546.
The single x2sys.py has been separated into x2sys_init.py
and x2sys_cross.py inside the src/ folder as per GenericMappingTools#807.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants