Incorporate Julia Putko's changes #86

hvdosser · 2022-06-15T16:45:21Z

Fixed conflicts in slocum.py and seaexplorer.py and removed Argo submodules.

Removed the module and the directory.

I've commented out some sections that Julia Putko added that may be causing tests to fail. These sections change the formatting for the dates, I think.

I made a typo in my fix, where I didn't comment out the end of a line. This is fixed here.

jklymak

OK, not sure about the pyglider changes. We need to move the website stuff out of pyglider and into cproofwebsite. I've not seen the plotting changes.

jklymak · 2022-06-15T18:54:32Z

pyglider/slocum.py

@@ -639,6 +639,113 @@ def merge_rawnc(indir, outdir, deploymentyaml,
        ds.to_netcdf(outnebd, 'w')


+    with open(deploymentyaml) as fin:


All these changes are reversing the new work we just did to clean this up, so please remove.

jklymak · 2022-06-15T18:55:49Z

pyglider/slocum.py

+#only the merge jpnote
+
+## jpnote: global variables ##
+# - preprocess overwrites variables -
+# - need global list to append multiple netcdf file attributes ## 
+
+globlist = []
+latmin = []
+latmax = []
+lonmin = []
+lonmax = []
+
+def preprocess(ds):
+
+    global globlist
+    global latmin
+    global latmax
+    global lonmin
+    global lonmax
+
+  #  dsnew = ds.copy()


You almost never want to use globals. I'm not sure what is being done here to be honest.

This was a rough fix for some of the things you later fixed properly. It's all been removed.

jklymak · 2022-06-15T18:57:31Z

pyglider/slocum.py

@@ -753,6 +867,25 @@ def raw_to_timeseries(indir, outdir, deploymentyaml, *,

        ds.attrs['deployment_start'] = str(start)
        ds.attrs['deployment_end'] = str(end)
+
+## jpnote: beginning of jp addition ## 


I can't tell what this is supposed to do, but I don't think it is correct.

It's been removed.

jklymak · 2022-06-15T18:58:49Z

pyglider/website.py

+# jpnote addition for argo float calcs 
+import argopandas as argo 
+import pandas as pd
+# end of jp addition 


Lets redo this PR with the changes in website, plotting, and other modules all separate. I think the website work all needs to move to the website repo? Definitely we cannot have argopandas in here.

These all look useful, but I think we need to move out of pyglider.

Everything argopandas related has been removed. For the website work, I'd like to first get it to merge properly into website.py, then move that into the website repo.

The argopanda commit is still in this PR...

For website.py, lets copy the version on upstream/main into cproofwebsite, and commit it there (maybe under scripts/ or something). Then make a pr on the website repo for the changes proposed here. The website can have argopandas as a dependency (not a submodule ;-)) and we can have the map plotting in there etc.

Probably the same with mapplotting.py. That is far too specific to our sites to be part of pyglider.

Again, we really want pyglider to be of general use, so we should try and move cproof specific routines out if we can.

Ah, yes that was an empty argopandas directory that's now been removed.

For website.py and mapplotting.py, can I finish this pull request and then move them over into cproofwebsite? It would be nice to not be trying to do two things at once, when I'm struggling with git for both of them.

I don't want to add mapplotting.py to git history of this repo (though its not a big deal). Just remove it from the PR, and move the file to a temperorary location.

We could merge the changes to website.py, but you may as well just do the same thing - move the version you want to use to a different location. Why? Because we know website.py works as-is. When we move it over, it is far better to start with a version that we know works, than one that hasn't been tested yet, and then make PRs on that version (i.e. the changes you propose here). The advantage is that we can always back the changes out.

jklymak · 2022-06-15T19:00:39Z

pyglider/seaexplorer.py

@@ -454,4 +454,3 @@ def _parse_sensor_config(filename):
    return active_device_dicts


-__all__ = ['raw_to_rawnc', 'merge_rawnc', 'raw_to_timeseries']


revert this

OK, but please add the extra line back in and remove the two spare carriage returns. We don't have linting turned on, but we will ;-). Liniting ensures some coding consistency and it will complain about both of these changes.

basically a little trick here is git restore --source=upstream/main pyglider/seaexplorer.py to restore the old version.

Please do the same for slocum below.

I've reverted an accidentally deleted line in seaexplorer.py and fixed a broken if statement in plotting.py

Removed Julia's fix that was doing similar things to Jody's more recent updates.

jklymak · 2022-06-15T20:50:12Z

pyglider/plotting.py

                                sharex=True, sharey=True)
        axs = axs.flat
        for n, k in enumerate(keys):
            _log.debug(f'key {k}')

            pconf = config['pcolor']['vars'][k]
            _log.debug(pconf)
-            cmap = pconf.get('cmap', 'viridis')
+
+            cmap = plt.cm.get_cmap('viridis') #jp


This change makes the colormap un-configurable...

Reverted to the original line.

jklymak · 2022-06-15T20:51:15Z

pyglider/plotting.py

-                    ax.xaxis.set_major_locator(locator)
-                    ax.xaxis.set_major_formatter(formatter)
+           #         locator = mdates.AutoDateLocator(minticks=3, maxticks=7)
+           #         formatter = mdates.ConciseDateFormatter(locator)


we probably want to keep this in here?

I put it back in.

Keeping this part of the code causes an out of index error in matplotlib/dates.py in the testing. I've looked at it, but I don't know enough about dates.py to troubleshoot it very effectively. Did you want me to just keep lines 270 and 271 or the entire block of code following those lines that Julia added (and similar pieces of code elsewhere in plotting.py?

We could probably figure out what is going on. But I'm confused because this has been woking, so I'm not clear why it is broken now.

Its because you didn't specify enough empty offsets.

But... why would you get rid of the offsets? The dates can often be meaningless with out them

You guys also changed a bunch of things about the plots that were features not bugs. You labeled all the ticks on each axes, which were explicitly not labeled in the original. You changed the date formatting, etc. I put substantial care into making the grid plots the way I want them. I'm happy to take suggestion on them, but I'll revert the ones here.

jklymak · 2022-06-15T20:51:34Z

pyglider/plotting.py

+            ax.yaxis.set_tick_params(labelbottom=True) 
+            ax.xaxis.set_tick_params(labelbottom=True)
+           # locator = mdates.AutoDateLocator() 
+           # formatter = mdates.ConciseDateFormatter(locator)


jklymak · 2022-06-15T20:52:17Z

pyglider/plotting.py

+                ax.set_ylim([300, 0])
+
+            vmax = 300
+            cmap.set_over("black") 


I'm not a huge fan of "over" being coloured black...

It was to differentiate it from the gray bathymetry. What colour would be better?

Actually I've just removed it because it was failing the tests.

OK - usually the over cover is just the max color of the colormap? Maybe you are referring to the NaN behaviour, which will just put an empty square. That is indeed hard to differentiate from bathymetry depth unless you have a bathy estimate from elsewhere.

jklymak · 2022-06-15T20:52:46Z

pyglider/plotting.py

                depth = ds.depth[:-1] - np.diff(ds.depth)
                depth = np.hstack((depth, depth[-1] + np.diff(ds.depth)[-1]))
                pc = ax.pcolormesh(time, depth, ds[k][:, ind],
-                    rasterized=True, vmin=min, vmax=max, cmap=cmap, shading='auto')
+                    rasterized=True, vmin=min, vmax=max,cmap=cmap, shading='auto')


The space will fail flake8

I put the space back.

jklymak · 2022-06-15T20:53:48Z

pyglider/plotting.py

+                                 ds[k].attrs['units'] + ']', fontsize=9)
+           # ax.set_title(ds[k].attrs['long_name'] + ' [' +
+                   #  ds[k].attrs['units'] + ']', loc='left', fontsize=9) #jpnote commented 
+            ax.set_title(ds[k].attrs['long_name'] + ' over Depth [m]', loc='left', fontsize=9)      


What does "over Depth [m]" mean?

jklymak · 2022-06-15T20:54:29Z

pyglider/plotting.py

+           # ax.set_ylim([depmax, 0])
+            ax.set_ylim([300, 0]) #jp changed (or should I leave it as dep max? )
+           # if n == 0:
+            ax.set_ylabel('DEPTH [m]')
+         #   ax.set_xlabel('TIME (May - June)')


I'd revert these changes...

jklymak

For the plotting, I think my take away is to not change the grid plots. I don't care about the time series plots as they looked pretty bad.

Removed the now-empty argopandas directory from pyglider directory.

Removed all jp changes from slocum.py and fixed an extra line and carriage returns in seaexplorer.py.

Undid some changes that would cause issues with colormaps, flake8, and date formatting.

Fixed an issue with axis labels and with the colormap.

jklymak · 2022-06-16T07:18:13Z

I'll merge this, even though its broken and push a new PR to fix

jklymak · 2022-06-16T08:01:36Z

Hi @hvdosser

So, this was confusing to deal with because I think you just made a PR on a c-proof branch versus your own branch? I see you have your own fork so the usual workflow would be:

clone your own fork locally:

git clone git@github.com:hvdosser/pyglider

If you do

$ git remote -v
origin  git@github.com:hvdosser/pyglider.git (fetch)
origin  git@github.com:hvdosser/pyglider.git (push)

you can see that your branch is the remote named origin.

Add upstream remote:

git remote add upstream git@github.com:pyglider/pyglider
$ git remote -v
origin  git@github.com:hvdosser/pyglider.git (fetch)
origin  git@github.com:hvdosser/pyglider.git (push)
upstream        git@github.com:c-proof/pyglider.git (fetch)
upstream        git@github.com:c-proof/pyglider.git (push)

Now there is a second remote named upstream. You can fetch it, and you will then have the commits in that remote on your machine:

git fetch upstream

Make a placeholder branch

This is less confusing than using main locally as your default branch. I do

$ git checkout -b placeholder
$ git reset --hard upstream/main

That makes the placeholder branch an exact copy of the upstream/main branch

Make a new branch

$ git checkout placeholder
$ git reset --hard upstream/main
$ git checkout -b my-new-branch

and edit away. However, make pull requests early and often. They are easier to review and check in about.

Commit some changes

Some code has changed, and you want to save it as a commit:

$ git commit -a -m "My description of code changes"

Push to GitHub remote

Do not push to upstream, unless you really really have to! Always push to origin!

$ git push origin my-new-branch

It should give a message like

remote: Create a pull request for 'fix-jp-changes' on GitHub by visiting:
remote:      https://github.com/hvdosser/pyglider/pull/new/my-new-branch

You can subsequently keep pushing to that branch as you add more commits.

Rebasing because upstream has changed

If upstream/main changes, then GitHub tries to reconcile your changes with the upstream changes. You can do this locally with

$ git fetch upstream
$ git rebase upstream/main

If there are no conflicts, then your changes do not conflict, and you can push up to origin no problem. However, because the base of the branch has changed from the old upstream/main to the new upstream/main you need to force push any changes:

$ git push origin my-new-branch --force

However, sometimes you will have "merge conflicts" because some one else has changed code you are also trying to change, and git can't figure out whose change to apply. The rebase will list these files with problems. For each one you need to

open the conflicted file in an editor. It will have markers like "<<<<<<<<<" in it denoting the conflicted regions. You need to look at this by hand and decide which change to keep, and of course edit out the markers.
After the file has all its conflicts reconciled, you need to re-add it $ git add pyglider/naughty_file.py

When all the files have been reconciled, you need to continue: $ git rebase --continue

Git reconciles each commit one by one, so its possible future commits will also have conflicts that also need to be reconciled. This is a good reason to keep your number of commits to a bare minimum.

Squashing commits

When developing, I'll often have a bunch of commits, but when it is time to reconcile with upstream, or push to GitHub. all these commits become a liability, so I can squash them. The most failsafe way to do this is to figure out the last commit that you did not author as part of the PR by doing git log.

commit 89805d1047677c87f76f41041c00fdadd016984e (HEAD -> fix-jp-changes, origin/fix-jp-changes)
Author: Jody Klymak <jklymak@gmail.com>
Date:   Thu Jun 16 09:32:14 2022 +0200

    Minor cleanups

commit ade50972704f190e7c72113ccfd31bc571dc144e
Author: Jody Klymak <jklymak@gmail.com>
Date:   Thu Jun 16 09:24:03 2022 +0200

    MNT: revert some things in jp plotting

commit cea08ae90af2be90ce237b416404330b10f03b3a
Author: Jody Klymak <jklymak@gmail.com>
Date:   Thu Jun 16 09:13:11 2022 +0200

    MNT: revert changes to grid plots

commit 6fcd7a7de7ac4a26a720eae6091a7c5d42de7cb8 (upstream/main)
Merge: eb46b71 0d3a214
Author: Jody Klymak <jklymak@gmail.com>
Date:   Thu Jun 16 09:18:19 2022 +0200

    Merge pull request #86 from c-proof/jp_dev
    
    Incorporate Julia Putko's changes

commit 0d3a2146450316ec5a4cad3edb24d8b5bb2aaa1b (upstream/jp_dev)
Author: C-PROOF <cproof@valdez.seos.uvic.ca>
Date:   Wed Jun 15 16:10:10 2022 -0700

    Reverting changes in plotting.py
    
    Fixed an issue with axis labels and with the colormap.

So here, the last commit that was on GitHub was 6fcd7a7de7. To squash these commits, I do

git rebase -i  6fcd7a7de7ac

which will do an interactive rebase. This should open my editor, and then I choose to "squash" or "pick" the commits. Int his case I'd squash all except cea08ae90a, which I'd "pick".

After you are done this you can force to your PR: git push origin my-new-feature --force. Note that you have to --force again because commits are being destroyed on the origin.

juliaputko and others added 6 commits April 26, 2022 19:21

pyglider additions, adding argopandas as submodule

4afbc5e

Removed argopandas from pyglider

cfedc65

Removed the module and the directory.

Merge branch 'main' of https://github.com/c-proof/pyglider into jp_dev

6b92f0e

Merge branch 'main' of https://github.com/c-proof/pyglider into jp_dev

2a5e8fd

Changing plotting.py to correct an issue with date offsets

24ed799

I've commented out some sections that Julia Putko added that may be causing tests to fail. These sections change the formatting for the dates, I think.

Typo in fix for plotting.py

89bd103

I made a typo in my fix, where I didn't comment out the end of a line. This is fixed here.

jklymak requested changes Jun 15, 2022

View reviewed changes

C-PROOF added 2 commits June 15, 2022 12:25

Addressing Jody's comments on the PR

54eb3a1

I've reverted an accidentally deleted line in seaexplorer.py and fixed a broken if statement in plotting.py

Removed some of Julia's changes from slocum.py

284f573

Removed Julia's fix that was doing similar things to Jody's more recent updates.

jklymak reviewed Jun 15, 2022

View reviewed changes

C-PROOF added 6 commits June 15, 2022 14:04

Removed the argopandas directory

15d37d4

Removed the now-empty argopandas directory from pyglider directory.

Reverting changes to seaexplorer.py and slocum.py

4e1b8c6

Removed all jp changes from slocum.py and fixed an extra line and carriage returns in seaexplorer.py.

Reverting changes to plotting.py

caace07

Undid some changes that would cause issues with colormaps, flake8, and date formatting.

Removed mapplotting.py and website.py from the PR

339093b

Removed cmap.set_over("black") from plotting.py

f516ee7

Reverting changes in plotting.py

0d3a214

Fixed an issue with axis labels and with the colormap.

jklymak merged commit 6fcd7a7 into main Jun 16, 2022

jklymak deleted the jp_dev branch June 21, 2022 13:10

		@@ -639,6 +639,113 @@ def merge_rawnc(indir, outdir, deploymentyaml,
		ds.to_netcdf(outnebd, 'w')


		with open(deploymentyaml) as fin:

		@@ -454,4 +454,3 @@ def _parse_sensor_config(filename):
		return active_device_dicts


		__all__ = ['raw_to_rawnc', 'merge_rawnc', 'raw_to_timeseries']

Incorporate Julia Putko's changes #86

Incorporate Julia Putko's changes #86

Conversation

hvdosser commented Jun 15, 2022

jklymak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvdosser Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

hvdosser Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

jklymak commented Jun 16, 2022

jklymak commented Jun 16, 2022

clone your own fork locally:

Add upstream remote:

Make a placeholder branch

Make a new branch

Commit some changes

Push to GitHub remote

Rebasing because upstream has changed

Squashing commits

hvdosser Jun 15, 2022 •

edited

Loading

hvdosser Jun 15, 2022 •

edited

Loading

jklymak Jun 15, 2022 •

edited

Loading