General PR Patch #138
General PR Patch #138
Conversation
…s + load cloud fix
|
hello @eleniv3d and @DamienGilliard , if you have the time to review or at least have a look and validate the new features would be great, no major breaking changes in it anyways. Cheers! |
There was a problem hiding this comment.
Hi @9and3 , Thanks a lot for this pr. I just added 2 general remarks, both of which I can implement if you want. I still approved the PR in case we leave it like that.
Cheers !
| ) | ||
| toggle.Attributes.ExpireLayout() | ||
| gh.Instances.ActiveCanvas.Document.AddObject(toggle, False) | ||
| ghenv.Component.Params.Input[indx].AddSource(toggle) # noqa: F821 | ||
|
|
||
|
|
||
| class DFCsvExporter(component): |
There was a problem hiding this comment.
Hi ! I didn't see a check on the number of character in a csv cell, to make sure the csv file is readable. Is it something that should be implemented later ? I can do it, no problem, but it's just to make sure we don't miss it. I guess we could simply add a warning to the component if the length of any of the "distances" of the row dictionnary is over the limit (for example 6553 distances if we keep 4 digits, because the limit is to 32767 in excel.
There was a problem hiding this comment.
there is none because right now, the way it is we will never export a cell with a large number of chars. That was the only case of the "distances" cell. With this refactoring we are exporting the distances separate, each distance in a different row. So no need for a check.
There was a problem hiding this comment.
Also we do not have CSV reader yet.
|
|
||
| self.sanity_check = [] | ||
| self.__serial_file_extenion: str = ".diffCheck" |
| raise e | ||
|
|
||
| @staticmethod | ||
| def load_pickle(file_path: str): |
There was a problem hiding this comment.
Another general question: apparently pickle is "not secure" because it can unpack and execute code without safeguards. Do we leave it like that, do we add a small warning like "only load .diffcheck files you trust" ? I don't have the answer so maybe we should ignore my remark. I can implement the addition of the warning if you want.
There was a problem hiding this comment.
Thanks for the remark, I think it's very pertinent what you are saying. A small constant RML grasshopper white box that does not disappear would actually not be enough. Let me see if I can use JSON instead without much work. Good point though!
description
Various fixes and new features from testing DF in the evaluation of AugmentedCarpentry experimental campaign.
new features
DFExportResults,DFImportResults,DFInspectResults: I added the serialization of the result object so that it can be saved locally under the extension.diffCheck, shared and reloaded. This was needed when evaluating multiple elements and not having the need to have a.ghfor each and re-run the results each time. This way this file can be lightweight and shared for repeatability.DFSaveCloudToPLY: simple as that.fixes
DF_cloud_cloud_distancesince it was broken but it was still included in the publicationDFCsvExportergh component distances export problem: we had a problem in exporting distances in one cell especially when exporting analysis data of large structures. In fact the limit of the csv of chars per cell is ~30k. To solve this problem, I added a separate toggle button to export distances as separate files but with id identification both in file naming and as a second column in the csv.DFCsvExportergh component as an extra crunchy perk 🥫DFLoadCloudFromFileinput checks to avoid errors + default scale value set to1.0fdocumentation