Casefile #574
Open
Casefile #574
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…also a hint of formatting style fix for case file name storage, for issue #7
…ly be for the 1st run/ninja, rather than letting it rename each run/ninja till the casefile ends with the final run/ninja casefile name rather than the 1st run/ninja casefile name. For issue #7
cleaned up last gui stuff including making WidgetDownloadDEM.cpp drop the casefile data structure declaration to make it follow in the behavior and style of stationFetchWidget.cpp, other whitespace and naming cleanup, slight alterations to the function ordering cleanup of casefile.h and casefile.cpp was mostly just whitespace and parenthesis syntax, though I also attempted to make some of the global static vars that were no longer needed globally, defined in a standard constructor way. Still could use cleanup in casefile.h/.cpp some tab to space corrections in ninja.cpp and ninjafoam.cpp
… up a pointer to the casefile in ninja/ninjafoam. Tested and it also works for diurnal. For issue #7
mostly just rearranging the casefile.h/.cpp to a slightly more coherent order before I start messing with variable names and editing functions for issue #7
removing the use and need of dirPath/directory in all the casefile stuff, after attempting to make sure that each of the dirPath/directory variables used everywhere in cli.cpp, mainWindow.cpp, ninja.cpp, and ninjafoam.cpp match up to be the same as well as match up to be the same as the case zip file directory also some more variable name changes, had to be careful though because zipFile was already being used as a variable name, and in particular was the class name of the mini_zip stuff this was surprisingly annoying to do, led to a lot of changes all over the place. But the result should make the code way more readable and easier to edit for future changes, including replacing existing functions with simpler cross-platform functions the bigger challenge is how cli.cpp is handling/setting the outputDir used by the zip file, it doesn't remain consistent in how it is set later in the code, kind of a chicken before the egg situation going on in that particular place in the code for issue #7
…unctions with cross-platform style functions
…iables and filenames, for issue #7 I'm still not the most happy with the new naming schemes, some of the names are starting to get a bit too long, and while they are now easier to differentiate from one another while more similar/consistent between files, some of them are still too similar to each other for my tests. Ah well, good enough for now.
rearranged some of the inputs to casefile in mainWindow.cpp and cli.cpp, including the way ofstreams were being opened and used. Needed ofstreams to stay in scope only opening when required, instead of always opening. Similar updates to the locations in the code for file deletions, only open files when needed and if deletion is required, delete right after closing. Cleanup of the error code in mainWindow.cpp as well, have to use qt forms instead of throw in the gui first attempt at getting the proper full list of wxModel times, when the user selects 0 times and so the code runs using all wxModel times. Still some todo/issues with this method dropped some unused variables, functions, and dependencies in casefile as well
moved split() and convertDateTimeToStd() from various places in the code to the casefile code itself. Also, replaced all instances of parse() with cross platform compatible CPLGetFilename(), CPLGetPath(). Also attempted to replace all path combining instances with "/" instances with CPLFormFilename()
got rid of split() function instances, by using cross platform compatible CPLGetFilename(), CPLGetPath(), and CPLFormFilename() functions instead. So cleanup of the station casefile stuff got rid of zipFile as input to various functions, just use the set value within the class figured out how to get the times right for the weatherModelInitialization with times.size() = 0 case, just required adding a getFullTimesList() style function in weatherModel.cpp cleaned up time stuff, including a change to the time facet to drop instances of ":" chars, windows does NOT like filenames with such ":" chars. General cleanup of time methods, as well as an attempt at doing a bit of cleanup to the addFileToZip() function, especially changing out the warning/error messaging functions that were used final general cleanup, dropped a few last unused #include header dependencies
…ed branch, for issue #7 most merging went fine automatically, it took the master branch form, adding in updated casefile stuff where different. But there were a few merge conflicts, in which cases I compared each version of the section, and took the best from both. This usually implied taking the master branch and editing it/adding to it the casefile stuff. In most cases, I think in all but just the ninjafoam.cpp vtk/mass mesh output sections, the two branch versions either replaced or appended lines to each other, without any edits combining a single line between the two branch versions.
…or issue #7 while it compiled, and ran in what ways it could, all the cpl_zip() type commands had to be commented out, so it isn\'t doing any zipping stuff anymore. The error is stuff like, "unresolved external symbol to cpl_zipOpen". So it seems like a change that needs to be made to CMakeLists.txt to include zlib. I see zlib inside the included GIS_INTERNALS, but it doesn\'t seem to be getting grabbed correctly. I tried building zlib from scratch, GIS_INTERNALS is using zlib 1.2.3 but I could only get ahold of the current zlib which was version 1.3.1. I\'m not sure if the problem was this zlib version difference, or if something else was missing, maybe some edits to how gdal is included to point to zlib or another path to something more specific in zlib, but including zlib this way didn\'t seem to fix the problem. Note that in casefile, the include is "#include "cpl_minizip_zip.h"", so maybe it\'s some other slightly missed include I think most of the other changes are good, except that the static_casts to get std::to_string(intVal) need to be checked, on both Ubuntu and Windows, to make sure that they don't mess up the printed values by adding decimals and stuff. Not sure what to do about "#include <mutex>", it wasn't Windows compatible. Seemed like it wasn\'t outputting the intermediate log files, might be a good thing, might not, need to check and make sure that the failing of the zip file to open is properly being handled in the later intermediate file outputs.
… broke a few of the time methods, plus a little bit of cleanup to the changes for building on windows commit. Tested and seems the static_cast<long long>(intVal) didn't seem to hurt stuff as much as I thought it would. For issue #7
…en" type problems for windows builds of the casefile. For issue #7 I was not successful at getting minizip to work on windows, the following changes didn't hurt Ubuntu builds they just showed more specifically the libs found behind the scenes for minizip, but it was required to even attempt to get minizip through zlib to work on windows. I specified the libraries to the GIS_INTERNALS locations, then later to a hand built older minizip (https://www.winimage.com/zLibDll/minizip.html to find https://github.com/zlib-ng/minizip-ng/tree/1.2) and a handbuilt zlib of version 1.2.8, neither method fixed the problem. Seems to imply that the issue isn't minizip in zlib by itself, but that minizip in zlib has to be built correctly into gdal itself, which doesn't seem to be done for our GIS_INTERNALS windows build. Using "dumpbin" on the ".lib" files in GIS_INTERNALS, I was able to find that there are minizip bindings somewhere in there, but apparently GDAL somehow isn't built with them, the linking is somehow broken. Because of this, I think that we are going to need to try some other zip method that is more compatible with our windows builds.
…hods, first pass with GDAL zipping methods. for issue #7 still had to keep #include <mutex>, windows issue causing lib, to get this style of zipping to work without seg faulting for the 5 wxModel data times, none selected, case on Ubuntu. Still haven't found a workaround for <mutex> with the Windows build the windows build is now compiling with zipping, but VSIRename\(\) is causing problems with the current setup. In particular, it doesn't seem to matter how the file paths are fed into VSIRename\(\), VSIRename\(\) will rename the zip file regardless of whether it has uniform or a mix of "/" or "\" style chars. But while VSIRename\(\) works on an open zip file on Ubuntu, VSIRename\(\) will fail on Windows unless the zip file is closed and reopened during the renaming process. Worse, while all the folders and files are added to the zip properly when VSIRename\(\) fails to rename the zip, when VSIRename\(\) succeeds in renaming the zip it somehow drops most of the files from within the zip. there are pathing problems that need to be addressed for the windows build, apparently the gui grabs the dem file with "C:/Location" "/" style chars, but CPLFormFilename\(\) puts the remaining parts of the path together with "\" style chars, mixing the path char type. In addition, the paths seen in the various files within the zip get to be "\" in style, need to make sure that style makes sense. My current method attempted to avoid reopening and reclosing the zip over and over at each addFileToZip\(\) instance, by just opening the zip once at the beginning, and closing the zip once at the end. It seemed to work, at least till I hit the windows issues, but I'm not sure whether it is wise to leave the zip file hanging open like that the whole time. It does look like the method will work though, once we get the zip file renaming and the <mutex> syncing type issues figured out. Will probably need to watch out for properly closing the zip for each instance of the program quitting early though.
rearranged casefile functions a bunch, to now only allow zip file renaming AFTER all the files have been added to it, in an attempt to avoid windows zip renaming problems now runs fine for the windows build, other than the <mutex> issues, really need to find a windows equivalent for <mutex>. Need to do some additional testing and cleanup of the casefile GDAL zipping methods before moving on though.
…and/or casefile output from failing after the first run seems like this was broken since adding in the massMeshVtk output methods, but luckily it wasn't added as a generic method for all users till we made vtk output standard in the momentum solver related to issue #7
…efile, OpenMP omp_lock_t, which turned out to already be present all over in the WindNinja code. For issue #7 I think that this was required and works, because various ninjas were running in parallel, each trying to access and add file data to the same casefile zip file all at the same time with this fix, as well as the last one, I tested the casefile methods like crazy on both Windows and Ubuntu. The casefile methods now work quite well in all the different kinds of ways, on both windows and Ubuntu.
amongst other cleanup, renamed zip files "_ninja.zip" instead of ".ninja", windows required renaming the zip files adding on a ".zip" to access and use the zip files, seemed better to just have the files already in that format rather than forcing the user to mess with the file naming
… the zip file at every return, throw, exit instance. The gui seems like it is good to go, seems to be working correctly even when clicking cancel now, but the cli might need some more of this. For issue #7
masonwillman
commented
Apr 9, 2025
Contributor
Author
There was a problem hiding this comment.
Tested the casefile and have some questions:
- The .cfg file that is generated by the casefile is not formatted properly for a WindNinja_CLI run. Is there a reason for this? I think we should have that capability.
- Should we rename the .cfg files to match the casefile names? Right now, they are just named config.cfg for each casefile.
- In the casefile issue, the GUI should prompt the user to change a duplicate name, and the CLI should append a random number. Currently, the casefiles are overwritten in both cases. Is this what we want?
I will continue testing, but overall, it seems the casefile is working well aside from these questions.
Member
|
Good questions @masonwillman.
This casefile work won't be incorporated until the new GUI release in version 4.0.0. Let's hold off on merging this to master until we have branch 3.12 up to date for the next point release. After that, we'll merge this PR and plan to have 4.0.0 as our next feature release. We can continue cleaning up in this PR until then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a new pull request for the updated casefile branch.