Open Lvulis opened 1 year ago
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.
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
ensure_single_inlet
work on copies so that the original dictionaries are not modified (copies should probably be returned?)compute_topologic_metrics
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 :)
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.
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)
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?
@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).
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.
@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
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.
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).Up to this point everything is fine except that delt doesn't contain a super apex node and has the 3 inlet nodes.
Running
compute_topologic_metrics
modifies delt in place and kills all but the widest inlet nodes:Question
add_super_apex
at any point?compute_topologic_metrics
modifies in place, but that only matters for the super apex case?