View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0003053 | OpenFOAM | Patch | public | 2018-08-24 12:56 | 2018-09-03 18:44 |
| Reporter | Juho | Assigned To | henry | ||
| Priority | low | Severity | feature | Reproducibility | N/A |
| Status | resolved | Resolution | fixed | ||
| Platform | Unix | OS | Other | OS Version | (please specify) |
| Product Version | dev | ||||
| Fixed in Version | dev | ||||
| Summary | 0003053: Function1: CSV reader enhancements | ||||
| Description | I've attached modified CSV.C and CSV.H with two changes: 1. Allow reference column index to be higher than highest component column index. The previous version did not work with such setup. Change on line 115 in the attached CSV.C: - label nEntries = max(componentColumns_); + label nEntries = max(refColumn_,max(componentColumns_)); 2. Added optional scaling coefficients for the reference and component values. Default is 1. Possible use cases: - Easy adjustment of function frequency or amplitude - Unit conversions, for ex. when reading material property tables | ||||
| Tags | No tags attached. | ||||
|
|
CSV.C (8,338 bytes) CSV.H (4,802 bytes) |
|
|
I am concerned that you have added scaling functionality directly into the CSV class rather than providing it in a more general way; surely this would be useful to apply to any Function1? Or is there a reason why it should only be applied to CSV? Note that there is already a "scale" Function1: Function1/Scale/Scale.H However this only scales the result of the function rather than "x", perhaps we should extend "scale" or provide an additional Function1 which scales "x" so that it can be applied to any Function1. |
|
|
I have applied the first part of the patch: commit 90c74d8c7c6959dd6e7dc00f9dce69e6acf0d116 Function1/CSV: Allow reference column index to be higher than highest component column index |
|
|
You are correct, "scale" provides the functionality for value and thus the scaling coefficient for the result is unnecessary. I was mainly interested in scaling "x" and just added it also for the result while I was there without recognizing that there already was a more general approach for the result. Like you propose, extending "scale" to the "x" would be a more general solution. |
|
|
Although, perhaps multiplication by constant (i.e. unit conversion) is common enough task when reading tables that it would be worthwhile to include it directly to the table readers (TableBase). User interface would be much more simple than when using "scale" and the generality of the "scale" implementation is probably not needed in most cases. Scaling of the "x" is also not that relevant for many of the other function as they already include parameters to that effect. |
|
|
If we include scaling an one of the Function1 implementations we should include it in all of them to provide a consistent interface. I am not clean on the clutter and maintenance overhead this introduces so would rather keep the scaling operation separate. Adding "x" scaling to "scale" or provide it in a separate Function1 should not be a big problem and then it can be used in conjunction with any of the current or future Function1 implementations; I don't see why it should be limited to tabulated data. |
|
|
Resolved by commit 3fc52f94d77b513927ab896f4465105db4223b09 |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2018-08-24 12:56 | Juho | New Issue | |
| 2018-08-24 12:56 | Juho | File Added: CSV.C | |
| 2018-08-24 12:56 | Juho | File Added: CSV.H | |
| 2018-08-30 10:25 | wyldckat | Assigned To | => henry |
| 2018-08-30 10:25 | wyldckat | Status | new => assigned |
| 2018-08-31 16:25 | henry | Note Added: 0010019 | |
| 2018-08-31 16:50 | henry | Note Added: 0010020 | |
| 2018-08-31 17:40 | Juho | Note Added: 0010022 | |
| 2018-08-31 19:19 | Juho | Note Added: 0010023 | |
| 2018-08-31 20:17 | henry | Note Added: 0010024 | |
| 2018-09-03 18:44 | henry | Status | assigned => resolved |
| 2018-09-03 18:44 | henry | Resolution | open => fixed |
| 2018-09-03 18:44 | henry | Fixed in Version | => dev |
| 2018-09-03 18:44 | henry | Note Added: 0010036 |
