grass.jupyter: Create BaseSeriesMap to remove redundancies in SeriesMap and TimeSeriesMap

[29riyasaxena]

Solves #3276

[petrasovaa]

This is a good start, but there is still a lot of overlap e.g. in show() and save() methods that should be addressed.

[29riyasaxena]

This is a good start, but there is still a lot of overlap e.g. in show() and save() methods that should be addressed.

Hello @petrasovaa,

Thank you for your suggestions. I've taken the necessary steps as per your recommendations. However, I'm encountering difficulties with my setup, specifically regarding the proper installation of GRASS on my PC. I'm utilizing WSL and attempting to install GRASS by following the instructions outlined in this guide.

Despite following the guide, I'm encountering errors during the execution of pytest, which displays a ModuleNotFoundError: No module named 'grass.script'. In an attempt to resolve this, I manually added the cloned repository path to PYTHONPATH, resulting in a new error, subprocess.CalledProcessError: Command '['grass', '--config', 'path']' returned non-zero exit status 1.

Subsequently, I attempted to reinstall the package, yet encountered another issue indicating grass.script not found. Upon investigating the configuration directory path, I received the error message CRITICAL Junto library not added to PATH. Install junto.

Could you please provide guidance on resolving these issues?

Thank you.

[echoix]

Just to make sure, you compiled doing

./configure
make
make install

Where there were flags to configure, and used sudo make install if rights are needed?

Then, to make sure everything is installed,

which grass

Returns where it is installed,
And

grass

Opens the command line greeting you inside grass.
Inside this calling Python should work as you'd imagine. It's the simplest way to use it as originally designed.

[29riyasaxena]

Just to make sure, you compiled doing

./configure
make
make install

Where there were flags to configure, and used sudo make install if rights are needed?

Then, to make sure everything is installed,

which grass

Returns where it is installed, And

grass

Opens the command line greeting you inside grass. Inside this calling Python should work as you'd imagine. It's the simplest way to use it as originally designed.

Yes @echoix, I have followed the same steps with flags to ./configure , however, while running pytest, I am still getting this error ModuleNotFoundError: No module named 'grass.script'. Here's a snippet:

[echoix]

I either use Gitpod which runs an Ubuntu container online, or use Ubuntu 22.04 in WSL when developing locally. I've been using WSL since the beginning. I assume you are not using the WSL1, but the WSL2 which is Hyper-V-based, essentially a Linux VM, with great Windows integration.
I pretty much understand what's going on, I've been through this and it really isn't easy to workaround these quirks, since the python parts of grass are sadly not installed like any other grass package.
What is known to work, is what's used in CI here:

grass/.github/workflows/pytest.yml

Lines 67 to 79 in 44ea597

	- name: Add the bin directory to PATH
	run: |
	echo "$HOME/install/bin" >> $GITHUB_PATH
	- name: Test executing of the grass command
	run: .github/workflows/test_simple.sh
	- name: Run pytest
	run: |
	export PYTHONPATH=`grass --config python_path`:$PYTHONPATH
	export LD_LIBRARY_PATH=$HOME/install/grass84/lib:$LD_LIBRARY_PATH
	pytest --numprocesses auto .

Note that we don't install in the normal prefix (we use $HOME/install), but it could be anywhere, that's not a problem.

But there's something particular in your setup (since using WSL) that will make your development a bit harder. Remember how WSL2 is simply a Linux VM? You have files that reside in your Linux installation, but can also access the Windows filesystem throughout the mounted drives in /mnt/c/ (for your C:\ drive), or /mnt/d/ (for your D:\ drive) and so on.
Here, you cloned, compiled and installed grass inside your Linux install, but then you go back and want to test some files you changed in another clone of GRASS inside your Windows filesystem. Apart from the fact that it will be way slower (its closer to a network drive, and a lot of translation happens), and that the line endings are different (git usually adapts the line endings for the platform, and commits back a normalized version), there's probably a lot of things that could go wrong (like the pyc compiled files are for Linux or windows?). You'd be better off having these files all inside the Linux filesystem, and simply copying from Windows to Linux won't magically solve the different line endings or different filesystem permissions concepts.

[29riyasaxena]

Thanks @echoix! I attempted to place the files in my Linux filesystem, but encountered the same error. Ultimately, I decided to reinstall my WSL, and everything worked smoothly thereafter.

[echoix]

Thanks @echoix! I attempted to place the files in my Linux filesystem, but encountered the same error. Ultimately, I decided to reinstall my WSL, and everything worked smoothly thereafter.

Great to hear! And it's not a bad thing either! That's why I love using Gitpod, (same as GitHub codespaces, but existed before). I'm fresh each time I start something new.

[petrasovaa]

I added couple of comments I ran into while testing the code.

[29riyasaxena]

I added couple of comments I ran into while testing the code.

Hi @petrasovaa, I have made the major following changes:

1. I've defined the render function within the BaseSeriesMap, from which TimeSeriesMap and SeriesMap can inherit.
2. In the save class, I've retained the shared functionalities within the BaseSeriesMap while placing the specific functionalities in the child classes.

I'd also like to inform you about an issue I encountered during my setup while running the test for SeriesMap. It resulted in an error message: AttributeError: module 'grass.jupyter' has no attribute 'SeriesMap'. Upon investigation, I discovered that my version was 8.2. In an attempt to resolve the issue, I updated to the latest version, 8.3.2dev (2024), but the error persisted. Upon examining the source code, I noticed that the SeriesMap file was missing in the releasebranch_8_3. I then switched to the main branch and recompiled my code, yet the issue persisted. Could you please assist me in resolving this?

[petrasovaa]

I'd also like to inform you about an issue I encountered during my setup while running the test for SeriesMap. It resulted in an error message: AttributeError: module 'grass.jupyter' has no attribute 'SeriesMap'. Upon investigation, I discovered that my version was 8.2. In an attempt to resolve the issue, I updated to the latest version, 8.3.2dev (2024), but the error persisted. Upon examining the source code, I noticed that the SeriesMap file was missing in the releasebranch_8_3. I then switched to the main branch and recompiled my code, yet the issue persisted. Could you please assist me in resolving this?

SeriesMap was added recently, it's only in the main branch, so you need to work with the latest code.

[petrasovaa]

I added couple of comments I ran into while testing the code.

Hi @petrasovaa, I have made the major following changes:

1. I've defined the render function within the BaseSeriesMap, from which TimeSeriesMap and SeriesMap can inherit.
2. In the save class, I've retained the shared functionalities within the BaseSeriesMap while placing the specific functionalities in the child classes.

Thank you, I am not sure I will be able to review it today, but I'll try

[echoix]

Upon examining the source code, I noticed that the SeriesMap file was missing in the releasebranch_8_3. I then switched to the main branch and recompiled my code, yet the issue persisted. Could you please assist me in resolving this?

The Python files being available to use as imports work after a make install, as it's when they are copied to the correct location and hierarchy to be able to be used.

Changing branches that much and having problems after that, especially if it is with Python files that are created from a C module binding, I would expect you might need to run

make clean
make libsclean
make distclean
./configure #(with all the flags)
make -j4
make install

[29riyasaxena]

make distclean

Thank @echoix! This worked out.

[echoix]

I would've also deleted the directory where I installed too, to make sure everything was clean (ie files that are not there anymore could not get removed by installing upon it)

[29riyasaxena]

I would've also deleted the directory where I installed too, to make sure everything was clean (ie files that are not there anymore could not get removed by installing upon it)

Did it already to avoid the chaos. ✌️

[29riyasaxena]

I'm encountering an issue while running tests locally on my PC. I've been referring to the testing source code and module and running the test framework documentation on the GRASS GIS website. In the /grass/python/grass/jupyter/tests directory, I attempt to execute the command python3 -m grass.gunittest.main --location /home/riya/grassdata --location-type nc, but it produces the following output:

Executed 0 test files in 0:00:00.011313.
From them 0 files (unknown percentage) were successful and 0 files (unknown percentage) failed.
No tests found or executed.

Could you please provide guidance on what might be causing this issue?

[wenzeslaus]

...In the /grass/python/grass/jupyter/tests directory, I attempt to execute the command python3 -m grass.gunittest.main --location /home/riya/grassdata --location-type nc, but it produces the following output:

Sorry, there is no way for you to know this, but there we are experimentally using pytest, so you need to run:

PYTHONPATH=$(~/Projects/grass/code/grass/bin.x86_64-pc-linux-gnu/grass --config python-path):$PYTHONPATH pytest .

To run the grass.gunittest, you just need to be in a parent directory for it to find the testsuite directory, so cd /grass/python/grass/jupyter. (You can do a quick search in the files to see what the tests cover - I think it does not cover any of the classes you are modifying.)

However, if you know Jupyter notebooks, it advantageous if you run the examples in doc/notebooks this will give you a good way to try things out and it may help you debug it. (For that you should check reloading imported Python modules.)

Whether you go with the pytest tests or the notebook, you need to run it in your development environment to shorten the feedback loop. (The CI shows the specific issues quite nicely, but locally compiling just the directory and testing the relevant code takes fraction of the time CI needs to complete.)

[petrasovaa]

Hi @petrasovaa, I have made the major following changes:

1. I've defined the render function within the BaseSeriesMap, from which TimeSeriesMap and SeriesMap can inherit.
2. In the save class, I've retained the shared functionalities within the BaseSeriesMap while placing the specific functionalities in the child classes.

Thank you, I am not sure I will be able to review it today, but I'll try

Didn't have time to look at this more closely today, but at this point it's best to focus on being able to test the changes and fixing the problems the CI found, it will make any further development and review much easier.

[29riyasaxena]

Hi @petrasovaa, I have made the major following changes:

1. I've defined the render function within the BaseSeriesMap, from which TimeSeriesMap and SeriesMap can inherit.
2. In the save class, I've retained the shared functionalities within the BaseSeriesMap while placing the specific functionalities in the child classes.

Thank you, I am not sure I will be able to review it today, but I'll try

Didn't have time to look at this more closely today, but at this point it's best to focus on being able to test the changes and fixing the problems the CI found, it will make any further development and review much easier.

Sure thing @petrasovaa. I'll prioritize testing the changes and addressing the CI findings by the end of today.

[29riyasaxena]

...In the /grass/python/grass/jupyter/tests directory, I attempt to execute the command python3 -m grass.gunittest.main --location /home/riya/grassdata --location-type nc, but it produces the following output:

Sorry, there is no way for you to know this, but there we are experimentally using pytest, so you need to run:

PYTHONPATH=$(~/Projects/grass/code/grass/bin.x86_64-pc-linux-gnu/grass --config python-path):$PYTHONPATH pytest .

To run the grass.gunittest, you just need to be in a parent directory for it to find the testsuite directory, so cd /grass/python/grass/jupyter. (You can do a quick search in the files to see what the tests cover - I think it does not cover any of the classes you are modifying.)

However, if you know Jupyter notebooks, it advantageous if you run the examples in doc/notebooks this will give you a good way to try things out and it may help you debug it. (For that you should check reloading imported Python modules.)

Whether you go with the pytest tests or the notebook, you need to run it in your development environment to shorten the feedback loop. (The CI shows the specific issues quite nicely, but locally compiling just the directory and testing the relevant code takes fraction of the time CI needs to complete.)

Thank you for the detailed instructions, @wenzeslaus! I've successfully run the pytest tests in the way mentioned by you. Additionally, I revisited the temporal.ipynb in the doc/notebooks directory for a deeper understanding. I'll try running other examples also.

[29riyasaxena]

@petrasovaa I have made one major change by defining an attribute is_date, which is true if the class is TimeSeriesMap; it is False.

All of the tests except the PythonCode Quality check got passed, which failed because I had defined 14 attributes instead of 12. Could you please guide me further?

[petrasovaa]

@petrasovaa I have made one major change by defining an attribute is_date, which is true if the class is TimeSeriesMap; it is False.

I will get back to you on this, I need myself more time to see what is the best way to proceed. The way you have it now is not how base classes typically look like, although it technically works. I appreciate you are working on this, it's perhaps a more difficult task than I envisioned.

[petrasovaa]

Please test it in a notebook, running pytest is not enough, it doesn't cover everything. I am getting errors when I try to run it for both timeseriesmap and seriesmap, something's wrong with self._indices.

[petrasovaa]

Please also remove all the commented old code.
Also please address the pylint complaint:
grass/jupyter/baseseriesmap.py:112:8: W0201: Attribute 'base_file' defined outside __init__ (attribute-defined-outside-init)

[petrasovaa]

To make the pylint check pass, increase the limit in the configuration, we don't want this to be too strict:

diff --git a/python/.pylintrc b/python/.pylintrc
index 0aed22e524..0df20a6e3b 100644
--- a/python/.pylintrc
+++ b/python/.pylintrc
@@ -568,7 +568,7 @@ max-args=12
 
 # Maximum number of attributes for a class (see R0902).
 # We tend to have classes with more attributes than the default 7.
-max-attributes=12
+max-attributes=15
 
 # Maximum number of boolean expressions in an if statement (see R0916).
 max-bool-expr=5