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

Working with a super apex #93

Open
Lvulis opened this issue Feb 23, 2023 · 8 comments
Open

Working with a super apex #93

Lvulis opened this issue Feb 23, 2023 · 8 comments

Comments

@Lvulis
Copy link
Collaborator

Lvulis commented Feb 23, 2023

The (old) script I'm using to extract DCN's is only semi-working to get out a super apex. Leaving a note here to improve documentation and also get a working example to have a super apex.

from rivgraph.classes import delta
from matplotlib import pyplot as plt
import numpy as np
import rivgraph.im_utils as im
import rivgraph.mask_utils as mu
from rivgraph.io_utils import write_geotiff
import os
from osgeo import gdal
from datetime import datetime
import sys
import geopandas as gpd
import rivgraph.ln_utils as lnu
import pandas as pd

name = 'Pearl'
tht = 70

fn = name + "/RG_0.3/thresh_" + str(tht)
path_results = os.path.join(delta_dir, fn)
fn = name + "/" + name + "_filled.tif"
path_mask = os.path.join(delta_dir, fn)

delt = delta(name, path_mask, path_results, verbose=True)
delt.compute_network()
delt.to_geovectors('network')

path_shrln = os.path.join(delta_dir, name + "/outlines/OAM_shoreline.shp")
path_inlet = os.path.join(delta_dir, name + "/outlines/" + name + "_inlet_nodes.shp")

delt.prune_network(path_shoreline=path_shrln, path_inletnodes=path_inlet,
                   prune_less=True)
delt.compute_link_width_and_length()
delt.assign_flow_directions()
delt.links, delt.nodes = lnu.add_artificial_nodes(delt.links, delt.nodes, delt.gdobj)
delt.compute_link_width_and_length() # Need to re-compute because we've split some links into 2 in order to add artificial nodes.
delt.to_geovectors('network')
# Plot the links with the directionality marked
delt.plot('directions')

delt.to_geotiff('directions')
adj_w = delt.adjacency_matrix(weight='wid_adj') # Can also weight by 'len_adj' or provide a vector of your own weights.
print(adj_w)

path_adj = os.path.join(delta_dir, name + "/w_adj.csv")

np.savetxt(path_adj, adj_w, delimiter=",")

delt.save_network()

The output json files visualized in QGIS on the Pearl delta, with green circles representing nodes, pink lines representing nodes, and 3 brown circles the inlet nodes. A random node is highlighted (red).
image

Up to this point everything is fine except that delt doesn't contain a super apex node and has the 3 inlet nodes.

len(delt.nodes['id'])
Out[16]: 238

delt.nodes.keys()
Out[18]: dict_keys(['idx', 'conn', 'id', 'inlets', 'outlets', 'arts'])

delt.nodes['inlets']
Out[19]: [955, 928, 1043]

Running compute_topologic_metrics modifies delt in place and kills all but the widest inlet nodes:

delt.compute_topologic_metrics()

delt.nodes.keys()
Out[22]: dict_keys(['idx', 'conn', 'id', 'inlets', 'outlets', 'arts'])

delt.nodes['inlets']
Out[23]: []

len(delt.nodes['id'])
Out[24]: 212

Question

  • What is RG's utility for returning the adjacency matrix modified to have a dummy row for the super apex? Where do I add add_super_apex at any point?
  • compute_topologic_metrics modifies in place, but that only matters for the super apex case?
@Lvulis
Copy link
Collaborator Author

Lvulis commented Feb 23, 2023

Just an update here:
Would be good to update the docs (https://veinsoftheearth.github.io/RivGraph/linksnodes/index.html) to include information on the super apex key for nodes.

@jonschwenk
Copy link
Collaborator

Running compute_topologic_metrics calls ensure_single_inlet which removes any inlets (and then dangling links) besides the widest one. This is currently hard-coded. I see three issues here to address

  • have ensure_single_inlet work on copies so that the original dictionaries are not modified (copies should probably be returned?)
  • allow the user to select the option to use the super apex method when calling compute_topologic_metrics
  • update documentation to reflect changes

I feel like we had the super-apex as a default at one point, but reverted for some reason. Not sure. In the meantime, what you should do to make progress is to just reconstruct compute_topologic_metrics for yourself, fixing these issues. i.e. copy the function into your analysis script, but change the parts of it to make sure it's working on copies and using the super-apex. If you get it working, you can make the changes on your cloned repo and open a pull request :)

@jonschwenk
Copy link
Collaborator

In short, we abstracted too much control from the user with the compute_topologic_metrics function and I'm suggesting that you take that control back yourself. We could also consider scrapping the function entirely and replace with examples/documentation of how to compute the metrics you want. That's probably ideal but dev time is limited round these parts.

@Lvulis
Copy link
Collaborator Author

Lvulis commented Feb 23, 2023

Got it. I have another issue laid in here:
Documenting how to correctly incorporate the super apex to begin with. Where should I call set_super_apex?

Would be happy to contribute to fixing up the function (I do think having an if/else type check if whether you're working with a super apex wouuld be relatively straightforward and happy to look into it)

@elbeejay
Copy link
Member

I like the conversation here and agree with it all (I think). As far as what is feasible to get done (vs what is idea), it might be simplest and easiest for us to take @Lvulis's suggestions and improve the documentation at this time, rather than try to make headway on code changes. What do you think @jonschwenk?

@jonschwenk
Copy link
Collaborator

@Lvulis Can you please upload an MWE so I can debug this? I have plenty of my own examples but it will be easier to work together here if we're working on the same one. I think all I need is your mask (plus the above code you've already provided).

@jonschwenk
Copy link
Collaborator

I like the conversation here and agree with it all (I think). As far as what is feasible to get done (vs what is idea), it might be simplest and easiest for us to take @Lvulis's suggestions and improve the documentation at this time, rather than try to make headway on code changes. What do you think @jonschwenk?

I think I can knock this out in the coming weeks--it really shouldn't be too heavy of a lift.

@Lvulis
Copy link
Collaborator Author

Lvulis commented Mar 10, 2023

@jonschwenk you can use the mask available via Drive below. I'd be happy to start working on this but it isn't at the top of the list, have been busy with wrapping up paperwork + the move. Are you looking to first document the super apex functionality, or to update the functionality per your original suggestion?

https://drive.google.com/file/d/1izX5heHVBlbkJY15gxHHgq850Wp7DUwc/view?usp=sharing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants