[com4]: output for release area information#1213
Conversation
PaulaSp3
added
enhancement
Analysis for project
|
| Tool | Category | Rule | Count |
|---|---|---|---|
| qlty | Structure | Deeply nested control flow (level = 4) | 5 |
| qlty | Structure | Function with many parameters (count = 16): __init__ | 1 |
| qlty | Structure | High total complexity (count = 66) | 1 |
| qlty | Structure | Function with high complexity (count = 16): mergeDict | 1 |
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
|
Coverage Impact ⬇️ Merging this pull request will decrease total coverage on Modified Files with Diff Coverage (4)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
PaulaSp3
force-pushed
the
PS_FP_outputRelInfo
branch
from
9d893b8 to
e75f10d
Compare
| fluxDistOldVersionBool=False, | ||
| FSI=None, | ||
| forestParams=None, | ||
| startcellVol=None, |
| if key in processedStartCellIdDict: | ||
| ids = np.append(processedStartCellIdList[i][key], processedStartCellIdDict[key]) | ||
| processedStartCellIdDict[key] = np.unique(ids) | ||
| else: |
| startCellIdDict[(cell.rowindex, cell.colindex)], startcellId) | ||
| startCellIdDict[(cell.rowindex, cell.colindex)] = np.unique( | ||
| startcellIdList) | ||
| else: |
| ) | ||
| else: | ||
| relVolMinArray[cell.rowindex, cell.colindex] = max( | ||
| relVolMinArray[cell.rowindex, cell.colindex], cell.startcellVolMin |
| else: | ||
| mergedDict[cellind] = smallDict[cellindSmall] | ||
| mergedDict[cellind] = np.unique(mergedDict[cellind]) | ||
| log.info("appended result %s_%i_%i", fName, i, j) |
| if cellind in mergedDict: | ||
| mergedDict[cellind] = np.append(smallDict[cellindSmall], mergedDict[cellind]) | ||
| else: | ||
| mergedDict[cellind] = smallDict[cellindSmall] |
PaulaSp3
force-pushed
the
PS_FP_outputRelInfo
branch
from
e75f10d to
e5091f8
Compare
| crs=demHeader["crs"], | ||
| ) | ||
| del pathPolygons | ||
| return gdfPathPolygons |
PaulaSp3
force-pushed
the
PS_FP_outputRelInfo
branch
from
e5091f8 to
ae29065
Compare
PaulaSp3
force-pushed
the
PS_FP_outputRelInfo
branch
from
ae29065 to
c1ae153
Compare
ahuber-bfw
left a comment
ahuber-bfw
left a comment
There was a problem hiding this comment.
Hey @PaulaSp3,
Cool features (esp. the releaseID to Polygon feature :) )
only some questions/remarks:
- maybe you can add some tests for the new functions in
splitAndMerge.pyin thetest_com4FlowPy.pyfile - handling of additional output arrays in
flowCore.py--> to be consistent with the method used for other optional outupts (e.g. infra, forestInteraction) maybe we could only initiate the required arrays, if the option is set in the .ini - else just assignNone? - regarding the option to have a
relVolMinandrelVolMaxoutput- are there use-cases, where this is interesting and you don't also use the relID option at the same time?
- if this is only used togehter with the relID option, then this could probably be implemented at a higher level or in a post-processing step without need for modification in
flowCore.pyandflowClass.py
| - ``depFluxSum``: deposited flux summed up over all paths | ||
| - ``travelLengthMin``: the travel length along the flow path (the minimum value of all paths for every raster cell) | ||
| - ``fpTravelAngleMin``: the gamma angle along the flow path (the minimum value of all paths for every raster cell) | ||
| - ``relVolMin``: the minimum of the tracked release volume that route through the raster cell (the volume is provided in the release file in the REL folder) |
There was a problem hiding this comment.
generell die Frage, wie man mit den extra input-files umgehen sollte, damit die Vorgehensweise konsistent bleibt (auch im Hinblick darauf, wie man es macht, falls in Zukunft noch zusätzliche inputs dazu kommen):
akutell:
- releaseId --> aktuell eigener releaseIDPath oder .tif in RELID\ ABER
- releaseVolume --> implizit in \REL bzw. releasePath
konsistent(er)?:
- releaseID und releaseVolume als extra layer in \REL mit fixer Vorgabe für fileName bzw. jeweils mit eigenem releaseIdPath und releaseVolumePath ODER
- \REL \RELID \RELVOLUME als eigene folder in inputs bzw. releasePath, releaseIdPath und releaseVolumePath
| forestPath = "" | ||
| cfgPath["forestPath"] = forestPath | ||
|
|
||
| # read release ID raster |
There was a problem hiding this comment.
Gleicher kommentar, wie in der Doku:
- Macht's Sinn auch den releaseVolume input in ein extra file zu geben anstatt implizit im release file?
- Wenn ja, dann würd hier auch noch ein entsprechender Input-Check sinnvoll sein
| return mergedRas | ||
|
|
||
|
|
||
| def mergeDict(inDirPath, fName): |
There was a problem hiding this comment.
maybe consider writing a test for the new functions in tests/test_com4FlowPy.py
| if parent.forestIntCount < (self.forestIntCount - self.isForest): | ||
| self.forestIntCount = parent.forestIntCount + self.isForest | ||
|
|
||
| def calc_startCellVol(self, startcellVolNew): |
There was a problem hiding this comment.
generally - what is the use-case of tracking volMin and volMax?
- is it size-classification on the merged model results?
- is there a use-case, where
volMinandvolMaxare interesting and you don't use therelIDoption at the same time?
overall the naming von volMin and volMax could be a bit misleading/confusing - mainly, because it is physiclly improbable, that the whole release Volume is routed through each cell of a modeled process zone (which maybe could be represented be some combination with the routing flux?).
If this functionality is always/only interesting in conjunction with also using relIDs for contiguous release areas, could you simply add the information about min and max volumes at the final merging step in com4FlowPy.py?
| if relVolBool: | ||
| relVolArray = release.copy() | ||
| else: | ||
| relVolArray = None |
There was a problem hiding this comment.
also here:
- does it make sense to provide release volume in a separate file and not implicitly in the release file?
| forestIntArray = np.ones_like(dem, dtype=np.float32) * -9999 | ||
| relVolMinArray = np.ones_like(dem, dtype=np.float32) * -9999 | ||
| relVolMaxArray = np.zeros_like(dem, dtype=np.float32) | ||
| processedStartCellIdDict = {} |
There was a problem hiding this comment.
only initialize these if the corresponding options in relOutputParams are set to True?
| travelLengthMaxList = [] | ||
| travelLengthMinList = [] | ||
| processedStartCellIdList = [] | ||
| relVolMinList = [] |
| if forestInteraction: | ||
| forestIntList.append(res[12]) | ||
| forestIntList.append(res[15]) | ||
| relVolMinList.append(res[13]) |
| travelLengthMinArray = np.ones_like(dem, dtype=np.float32) * -9999 | ||
| travelLengthMaxArray = np.ones_like(dem, dtype=np.float32) * -9999 | ||
|
|
||
| relVolMinArray = np.ones_like(dem, dtype=np.float32) * -9999 |
There was a problem hiding this comment.
only initialize if option is set, else initialize with None?
in this way we do not reserve any memory if the array is not used in the next steps + also the return statement (lines 813 - 830) at the end of the function could stay like it is (all possible results are returned, but the ones which are not calculated are set to None - higher level functions can then also easily check, if a give result is available ....
| fluxDistOldVersionBool=fluxDistOldVersionBool, | ||
| FSI=forestArray[row_idx, col_idx] if isinstance(forestArray, np.ndarray) else None, | ||
| forestParams=forestParams, | ||
| startcellVol=startcellVol, |
There was a problem hiding this comment.
see comments in flowClass.py
--> is it really necessary to implement this at the level of single cells (since it only passes one particular information from release cell to all the cells in the process area, if I understand correctly?)
- maybe this can be accomplished at a higher level or in a quick post-processing step?
| cfgPath["tempDir"] = pathlib.Path(temp_dir) | ||
| cfgPath["demPath"] = pathlib.Path(cfgCustomPaths["demPath"]) | ||
| cfgPath["releasePath"] = pathlib.Path(cfgCustomPaths["releasePath"]) | ||
| cfgPath["relIdPath"] = pathlib.Path(cfgCustomPaths["relIdPath"]) |
There was a problem hiding this comment.
im com4FlowPyCfg.ini heißt die variable releaseIdPath --> wird deshalb hier so nicht gefunden
| workDir = | ||
| demPath = | ||
| releasePath = | ||
| releaseIdPath = |
There was a problem hiding this comment.
im runCom4FlowPy.py wird nach relIdPath gesucht --> hier oder dort anpassen damits funktioniert
still in progress relVolMin and relVolMax as output correct position for relId delete print small change fix bug pathPolygons as output rename function tidy up try to keep RAM as small as possible print a warning if the output in cfg is not completly valid small corrections correct bugs and doc update docu adapt pytest delete unused package
…or releaseID functionality
PaulaSp3
force-pushed
the
PS_FP_outputRelInfo
branch
from
4f4a576 to
8b69944
Compare
2 participants
rasters are only merged if they are written as output.
computation time (for my example): +25%
new outputs:
required input data (for these outputs):