Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the EN_loadpatternfile function to load pattern data from a .pat file into INP #812

Merged
merged 5 commits into from
Aug 6, 2024

Conversation

Mariosmsk
Copy link
Member

No description provided.

Copy link
Collaborator

@LRossman LRossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the liberty of re-writing your EN_loadpatternfile function. It avoids reallocating memory each time a new line is read from the file and uses EPANET's own getfloat function in place of the unreliable atof function to convert strings to numbers. Please see the attached file.

EN_loadpatternfile.txt

Mariosmsk and others added 2 commits August 1, 2024 08:58
@Mariosmsk
Copy link
Member Author

It's good that you took the initiative, @LRossman. Feel free to make any changes as needed. Thank you! Also, we now need to add the function EN_savepatternfile.

@LRossman
Copy link
Collaborator

LRossman commented Aug 1, 2024

I was thinking, why have the function work only for a new pattern and not for an existing one as well. If you agree with this, then the code modification needed is shown below:

    file = fopen(filename, "r");
    if (file == NULL) return 302;

    // Add a new pattern if it doesn't already exist
    err = EN_getpatternindex(p, id, &i);
    if (err == 205) {
        if ((err = EN_addpattern(p, id)) != 0) {
            fclose(file);
            return err;
        }
        i = p->network.Npats;
    }

    // Read pattern values

@Mariosmsk
Copy link
Member Author

Yes, it's a good idea to use an existing pattern ID not only for new patterns, and I agree.

Regarding the getfloat function, there seems to be an issue in EN_loadpatternfile (error 205). So, I made the changes below in getfloat; however, I'm not sure if I have broken something else. What do you think?

from

{
    char *endptr;
    *y = (double)strtod(s, &endptr);
    if (*endptr > 0) return 0;
    return 1;
}

to

{
    char *endptr;
    *y = strtod(s, &endptr);

    if (endptr == s || (*endptr != '\0' && !isspace((unsigned char)*endptr))) {
        return 0;
    }
    return 1;
}

@LRossman
Copy link
Collaborator

LRossman commented Aug 1, 2024

I would rather not change getfloat since it's used extensively when processing an input file. If EN_loadpatternfile is returning an error 205 then it seems that either:

  1. The 205 error from EN_getpatternindex is not being reset to 0 before a new pattern is created.
  2. The values array passed into EN_setpattern is NULL implying that there's a problem with reallocating its size as the file is read.

Neither of these involves getfloat. I will download your code onto my machine and try to get a better understanding of why its failing.

@Mariosmsk
Copy link
Member Author

I checked it, and the problem is that the len=0. I believe the code skipped all the lines, so when I changed the line with getfloat, it worked.

@LRossman
Copy link
Collaborator

LRossman commented Aug 1, 2024

OK, good. My preference is to change the following line:

    err = EN_setpattern(p, i, values, len);

to

    if (len > 0) err = EN_setpattern(p, i, values, len);

@Mariosmsk
Copy link
Member Author

Mariosmsk commented Aug 1, 2024

But if we add len > 0, I think it doesn't do anything to set the patterns. The problem is that the code doesn't execute the line len++; so the length stays at zero. Maybe I am doing something wrong. I think it's good to test it.

@LRossman
Copy link
Collaborator

LRossman commented Aug 1, 2024

The way I read the code is that if a line is blank or doesn't contain a valid number then getfloat will return 0 and that line will be skipped. If that happens for all lines (or the file is empty) then the values array stays at NULL and len remains at 0. If you try to call EN_setpattern with a NULL ``values array it will return with error 205. Therefore testing if len > 0 before calling EN_setpattern will avoid that happening. And if len = 0 then the pattern's multipliers will remain unchanged.

But you're right in saying that the code needs to be tested.

@LRossman
Copy link
Collaborator

LRossman commented Aug 1, 2024

I think I found the problem. When getfloat is applied to line it sees the newline character at the end and therefore thinks the number contained in the line is invalid. (The reason we shouldn't simply use atof is that it won't catch an invalid number caused by a typo, e.g. it would read "123X.45" as "123".) When getfloat is used to read values from the input file lines have already been parsed into string tokens without any newline characters so it works fine in that case.

The fix is as follows:

    char *tok;
    ....
    // Read pattern values
    while (fgets(line, sizeof(line), file) != NULL) {
    
        // Skip lines that don't contain valid numbers
        tok = strtok(line, SEPSTR);
        if (tok == NULL) continue;
        if (!getfloat(tok, &value)) continue;

        // Resize multiplier array if it's full

I tested this updated code on a number of pattern files, some with all numbers, some with blank or comment lines and some empty, and everything seemed to work ok.

Co-Authored-By: Lew Rossman <[email protected]>
@Mariosmsk
Copy link
Member Author

Yes, it looks good now. Thank you, @LRossman!

@LRossman LRossman merged commit fbd005d into OpenWaterAnalytics:dev Aug 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants