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

Attempting to address 153, by allowing user to specifically import 99%,99.6%, 0.4%,1% and 2% annual design days #791

Open
wants to merge 40 commits into
base: develop
Choose a base branch
from

Conversation

antonszilasi
Copy link

@antonszilasi antonszilasi commented Feb 16, 2025

WIP to address issue 153 and sketching out some ideas

Copy link
Contributor

github-actions bot commented Feb 16, 2025

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@antonszilasi antonszilasi changed the title Dev/anton/153 WIP to address issue 153 and sketching out some ideas Feb 16, 2025
@macumber
Copy link
Collaborator

Thanks @antonszilasi! I think the first thing to do is to create a mockup of the UI that we want to make, the unmethours post suggested this from Trace:

image

Once we have the mockup, we can create the widget to match it.

@macumber
Copy link
Collaborator

macumber commented Feb 16, 2025

What about something like below:

image

We would look through the file and identify all the design day types and levels, then group them in a table. Each row would allow selecting one radio button. Each column would have a select all button to select all in that column

@antonpsd
Copy link

image

@jmarrec jmarrec marked this pull request as ready for review February 27, 2025 15:45
@jmarrec
Copy link
Collaborator

jmarrec commented Feb 27, 2025

Marking the PR as non-draft because I want a CI run on it. @antonszilasi I fixed up the clang format and the cppcheck thingy quickly.


std::vector<model::DesignDay> filterDesignDays(const std::vector<model::DesignDay>& designDays, const std::string& dayType,
const std::string& percentage, const std::string& humidityConditionType = "") {
std::vector<model::DesignDay> filteredDesignDays;
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably reserve first.

I think you could just do it in place instead of copying around though.

Copy link
Author

@antonszilasi antonszilasi Feb 28, 2025

Choose a reason for hiding this comment

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

@jmarrec

do you mean something like this?

filteredDesignDays.reserve(designDays.size());

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes

Copy link
Author

@antonszilasi antonszilasi Mar 1, 2025

Choose a reason for hiding this comment

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

@jmarrec
done here b76aeff

dialog.accept();
});

// Populate table for Heating and Cooling
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd expect a check to not include the checkboxes if the design days aren't in the DDY file to begin with.

Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines 716 to 731
connect(checkBox, &QCheckBox::toggled, [=, &designDaysToInsert](bool checked) {
if (checked) {
std::string dayType = (row == 0) ? "WinterDesignDay" : "SummerDesignDay";
std::vector<model::DesignDay> filteredDays = filterDesignDays(allDesignDays, dayType, percentages[col]);
designDaysToInsert.insert(designDaysToInsert.end(), filteredDays.begin(), filteredDays.end());
} else {
// Remove unchecked design days
std::string dayType = (row == 0) ? "WinterDesignDay" : "SummerDesignDay";
std::vector<model::DesignDay> filteredDays = filterDesignDays(allDesignDays, dayType, percentages[col]);
for (const auto& day : filteredDays) {
auto it = std::find(designDaysToInsert.begin(), designDaysToInsert.end(), day);
if (it != designDaysToInsert.end()) {
designDaysToInsert.erase(it);
}
}
}
okButton->setEnabled(!designDaysToInsert.empty());
});
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could avoid constantly refiltering, delaying the filtering until the "Ok" button is clicked.

Copy link
Author

Choose a reason for hiding this comment

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

@jmarrec
Copy link
Collaborator

jmarrec commented Feb 27, 2025

What about something like below:

image

We would look through the file and identify all the design day types and levels, then group them in a table. Each row would allow selecting one radio button. Each column would have a select all button to select all in that column

I like that a lot.

@jmarrec
Copy link
Collaborator

jmarrec commented Feb 27, 2025

I started a test bed here in case it's useful: https://godbolt.org/z/rxern817h

@antonszilasi
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Feb 28, 2025
{
return false;
}

Copy link
Author

Choose a reason for hiding this comment

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

@jmarrec somehow your other comments were lost but I addressed two of them in

78291c1

and a5263ba

@antonszilasi
Copy link
Author

antonszilasi commented Mar 1, 2025

Importing ddy which is missing 0.4% design days
https://github.com/user-attachments/assets/d654ae29-e2cd-4267-ae15-9d34aedc9ce6

@antonszilasi antonszilasi changed the title WIP to address issue 153 and sketching out some ideas Attempting to address 153 Mar 1, 2025
@@ -794,7 +794,7 @@ void InspectorGadget::layoutComboBox(QVBoxLayout* layout, QWidget* parent, opens
idx = 0;
combo->insertItem(idx, QPixmap(":/images/alert_image.png"), curVal.c_str(), "Invalid");

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what happened here, theres no change

Copy link
Collaborator

@macumber macumber Mar 1, 2025

Choose a reason for hiding this comment

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

There's a white space change on line 797, I bet your editor is set to automatically update the indents. You can just git checkout origin/develop -- src/model_editor/InspectorGadget.cpp

Copy link
Collaborator

Choose a reason for hiding this comment

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

I applied that fix and oddly this still shows up here, that is weird.

@antonszilasi antonszilasi changed the title Attempting to address 153 Attempting to address 153, by allowing user to specifically import 99%,99.6%, 0.4%,1% and 2% annual design days Mar 1, 2025

static QString key(const QString& type, int sortablePermil);

QString type() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmarrec it might be useful to add this interface directly to the SDK DesignDay object, basically it would just parse this information out of the DesignDay name. What do you think? Is that useful enough I should make a PR to the SDK?

@macumber
Copy link
Collaborator

macumber commented Mar 2, 2025

I'm trying to be nice and include GIFs :-) You can see there is some existing ugliness in the DesignDayGridView when deleting DDYs on the Location tab (it doesn't fully delete/hide the row), we could try to clean that up in this PR:

ImportDDYs

@jmarrec
Copy link
Collaborator

jmarrec commented Mar 4, 2025

I started a test bed here in case it's useful: https://godbolt.org/z/rxern817h

I think your interface is nice as is @macumber. I was originally thinking we've do something fancier/more explicitly per this link I started, but I don't know if it's worth overdoing it.

    std::vector<std::string> heatingPercentages = {"99.6%", "99%"};
    std::vector<std::string> coolingPercentages = {"2%", "1%", ".4%"};

    std::vector<DesignDayInfo> designDayTypes = {
        // Heating
        {.day_type = DesignDayInfo::DayType::Heating, .lookup_string = "DB", .name = "Heating", .tooltip = "Sizing heating equipment"},
        {.day_type = DesignDayInfo::DayType::Heating, .lookup_string = "WS=>MCDB", .name = "Heating Windy", .tooltip = "Used for sizing based on infiltration"},
        {.day_type = DesignDayInfo::DayType::Heating, .lookup_string = "DP=>MCDB", .name = "Heating Humid", .tooltip = "Used for sizing dehumidification systems"},

        // Cooling
        {.day_type = DesignDayInfo::DayType::Cooling, .lookup_string = "DB=>MWB", .name = "Cooling", .tooltip = "Sizing Chillers or air-conditioning units"},
        {.day_type = DesignDayInfo::DayType::Cooling, .lookup_string = "WB=>MDB", .name = "Cooling Latent", .tooltip = "Used to size cooling towers, evap-coolers, and fresh-air ventilation systems"},
        {.day_type = DesignDayInfo::DayType::Cooling, .lookup_string = "DP=>MDB", .name = "Cooling Dehum", .tooltip = "Used for sizing dehumidification systems"},
        {.day_type = DesignDayInfo::DayType::Cooling, .lookup_string = "Enth=>MDB", .name = "Cooling Enthalpy", .tooltip = "Used for sizing based on infiltration and/or ventilation"},

    };

It would look kinda like your mockup but with extra rows for the cooling ones and tooltips

image

setStyleSheet("QWidget#OSCellWrapper { border: none; border-right: 1px solid gray; border-bottom: 1px solid gray; }"
"QWidget#OSCellWrapper[header=\"true\"]{ border: none; border-top: 1px solid black; border-right: 1px solid gray; border-bottom: 1px "
"solid black; }");
setStyleSheet(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@antonszilasi These border lines in the stylesheet were what remained when we hid the cells

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants