View Issue Details

IDProjectCategoryView StatusLast Update
0003053OpenFOAMPatchpublic2018-09-03 18:44
ReporterJuhoAssigned Tohenry 
PrioritylowSeverityfeatureReproducibilityN/A
Status resolvedResolutionfixed 
PlatformUnixOSOtherOS Version(please specify)
Product Versiondev 
Fixed in Versiondev 
Summary0003053: Function1: CSV reader enhancements
DescriptionI'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
TagsNo tags attached.

Activities

Juho

2018-08-24 12:56

reporter  

CSV.C (8,338 bytes)
CSV.H (4,802 bytes)

henry

2018-08-31 16:25

manager   ~0010019

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.

henry

2018-08-31 16:50

manager   ~0010020

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

Juho

2018-08-31 17:40

reporter   ~0010022

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.

Juho

2018-08-31 19:19

reporter   ~0010023

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.

henry

2018-08-31 20:17

manager   ~0010024

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.

henry

2018-09-03 18:44

manager   ~0010036

Resolved by commit 3fc52f94d77b513927ab896f4465105db4223b09

Issue History

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