Skip to content

fromExcel Enhancements and add dataset.toDict#48

Merged
paul-griffith merged 14 commits into
IgnitionModuleDevelopmentCommunity:mainfrom
JosephTLockwood:main
Jun 28, 2023
Merged

fromExcel Enhancements and add dataset.toDict#48
paul-griffith merged 14 commits into
IgnitionModuleDevelopmentCommunity:mainfrom
JosephTLockwood:main

Conversation

@JosephTLockwood

Copy link
Copy Markdown

This pull request would add two new features to the fromExcel function.

  1. Fix the error of having a table with different row lengths throwing a null pointer exception.
    Use case example
PartNumber Desc
44 This is part 44
4A

Because the Desc is blank for 4A, this would throw a null pointer exception.

  1. Allow you to import specified columns as strings.
    Use case example
PartNumber Desc
44 This is part 44
4A

Because 44 is the first value, the table column would be assigned Int::class.javaObjectType; this would cause 4A to throw an error. I added a new parameter named stringColumns. stringColumns takes in a list of column positions to String.

I am new to GitHub (I don't know how to merge this properly) and Kotlin (code may not be as precise as needed). The files changed where

  • common\src\main\kotlin\org\imdc\extensions\common\DatasetExtensions.kt
  • common\src\main\resources\org\imdc\extensions\common\DatasetExtensions.properties

@JosephTLockwood JosephTLockwood changed the title fromExcel Enhancements fromExcel Enhancements and add dataset.toDict Jun 27, 2023
@JosephTLockwood

Copy link
Copy Markdown
Author

Added toDict. This would be useful for multiple applications, such as separating a dataset by columns and extracting values into an array.

fromExcel(
"input",
"headerRow",
"sheetNumber",
"firstRow",
"lastRow",
"firstColumn",
"lastColumn",
"stringColumns" = (new) Integer[]
)

toDict(
"dataset", = (new) Dataset
"filterNull", = (new) Boolean
)

@paul-griffith

Copy link
Copy Markdown
Member

Very cool!

I'm definitely on board with enhancements/bug fixes. I think an approach that might more sense than a strict "string these columns" override would be the ability to specify any type overrides? Maybe in a dictionary form of {colIndex: type}? That way you could bypass any of the autodetection logic entirely.

If you're not using IntelliJ, I'd recommend it. If you install the Ktlint plugin, it'll help clean up the formatting issues that will currently fail this PR if I run the github actions. In addition to that, we'll want to clean up the unnecessary committed bin/ files - not sure where those came from. There's also some improvements to be made on the toDict function to make it a bit faster.

If you're not comfortable making any of those changes, I'm happy to steer this PR a little - I really appreciate anyone making contributions, and don't want to bog you down with process :) If you do want to try, I'm also happy to help you out with the process.

@JosephTLockwood

Copy link
Copy Markdown
Author

Hey, thanks so much for the response. I currently use VSCode, so there is no formatting or code feedback. It has been a lot of googling, compiling, and troubleshooting. I'll plan on setting up a better environment to code tomorrow. Would you happen to have any recommendations for clearing the bin files besides deleting the folder? I am currently running the following commands when I want to create a new .modl file.

  • gradlew build clean
  • gradlew zipModule

Feel more than free to help with this. I would appreciate it. Today is day two of Kotlin for me. You won't be stepping on any toes here 😂

I like your suggestion of {colIndex: type}. It makes the most logical sense and was how I first approached it. I got overwhelmed and aborted it (that was day one). I'll give it a shot tomorrow if I have time. I am feeling more confident now.

I am unsure what to optimize in toDict to make it "faster". I thought of switching to ArrayList to prevent reallocations (I hate that I am doing toList every loop). I couldn't get it to work, though. It always seemed to then return a PyDictionary with Nulls or Units (depending on how I coded it). I just learned about the Kotlin filterNotNull, so I plan to try something like this tomorrow.

    for (column in 0 until dataset.columnCount) {
        val rowArray = (0 until dataset.rowCount)
            .map { row ->
                dataset.getValueAt(row, column)
            }
            .let { values ->
                if (filterNull) {
                    values.filterNotNull()
                } else {
                    values
                }
            }
            .toList()
        
        pyDict[dataset.columnNames[column]] = rowArray
    }

Once again, thanks for the response. I love this project and think it is a great idea; hopefully, I can help.

@paul-griffith

Copy link
Copy Markdown
Member

I am currently running the following commands when I want to create a new .modl file.

Those should be the correct commands, but gradle uses the build/ directory, not bin/ - maybe it's something in VSCode creating those directories. I'll again highly recommend IntelliJ here :) It's unfortunate, and possibly my biggest problem with Kotlin the language - it's created by Jetbrains who also make IntelliJ, so IntelliJ is by far and away the best way to author Kotlin code.

I went away and cleaned up the PR - I dropped the bin/ files that were added and tweaked the implementations of toDict and fromExcel. If you're able to run these and test them (or provide me any files you're using to test so I can add more unit tests) that would be much appreciated. If you'd like me to go into further detail on the changes I've made, I'm also happy to do that :)

Comment on lines +242 to +251
dataset.getColumnName(col) to PyList(
buildList {
for (row in dataset.rowIndices) {
val value = dataset[row, col]
if (value != null || !filterNull) {
add(value)
}
}
},
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Technically this allocates one unnecessary Pair instance for each iteration, but since the actual heavy object here will be the list of column data, I think this is fine. getColumnAsList would be helpful here, but then you'd have to filter the list as a second operation, meaning two scans of the same column instead of just one.

Comment thread common/src/main/kotlin/org/imdc/extensions/common/DatasetExtensions.kt Outdated
@JosephTLockwood

Copy link
Copy Markdown
Author

Okay, so I ran into an issue I can't solve being typeOverrides being declared. I also make a test case (I believe is working a sanity check is most likely in order). When you have a second would appreciate you looking at it. Let me know if the formatting is still incorrect. I downloaded Ktlint, but am not sure if it is working.

@JosephTLockwood

JosephTLockwood commented Jun 28, 2023

Copy link
Copy Markdown
Author

Wouldn't we want the parameter to be a PyStringMap instead of a PyDictionary?

@JosephTLockwood

JosephTLockwood commented Jun 28, 2023

Copy link
Copy Markdown
Author

Test case is passing now. It was an issue with toIndex() and the fact that I forgot that I needed a headerRow=0 for my test case.

@paul-griffith

Copy link
Copy Markdown
Member

I think typeOverrides will have to accept a dictionary where the keys are just column indexes (within the clump of data to be parsed). Using string names is too fragile.

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